Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ruby/spec changes to future proof for 64-bit fixnums on 64-bit Windows #11130

Merged
merged 4 commits into from
Jul 24, 2024

Conversation

XrXr
Copy link
Member

@XrXr XrXr commented Jul 9, 2024

See discussions in [Bug #20614] https://bugs.ruby-lang.org/issues/20614#note-9

See commentary in each commit.

@XrXr XrXr marked this pull request as draft July 9, 2024 01:19
@XrXr

This comment was marked as outdated.

@XrXr XrXr force-pushed the fix-integer-size-method branch 2 times, most recently from 7f6b083 to fddb332 Compare July 23, 2024 22:32
@XrXr XrXr changed the title Make Integer#size return SIZEOF_VALUE for fixnums ruby/spec changes to future proof for 64-bit fixnums on 64-bit Windows Jul 23, 2024
@XrXr XrXr requested a review from eregon July 23, 2024 22:38
@XrXr XrXr marked this pull request as ready for review July 23, 2024 22:38
@@ -62,6 62,12 @@ def self.wsl?
WORD_SIZE
end

C_LONG_SIZE = if defined?(RbConfig::SIZEOF[])
Copy link
Member

@eregon eregon Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what WORD_SIZE actually means given there is already POINTER_SIZE.
Maybe WORD_SIZE is meant to be sizeof(long) already but with a bad name?
Do you think it would make sense to basically rename :wordsize to :c_long_size (or make it an alias of it and still update all usages)?

Copy link
Member Author

@XrXr XrXr Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! In fact, it looks like the only usage of wordsize? left is platform_spec. I've pushed code to rename it.

XrXr added 4 commits July 24, 2024 10:38
…r#size

What a "word" is when talking about sizes is confusing because it's a
highly overloaded term. Intel, Microsoft, and GDB are just a few vendors
that have their own definition of what a "word" is. Specs that used the
"wordsize" guard actually were mostly testing for the size of the C
`long` fundamental type, so rename the guard for clarity.

Also, get the size of `long` directly from RbConfig instead of guessing
using Integer#size. Integer#size is not guaranteed to have anything to
do with the `long` type.
There is no guarantee that Integer#size will continue to return
`sizeof(long)` for small integers.

Use the `l!` specifier for Array#pack instead. It is a public
interface that has a direct relationship with the `long` type.
It's better than guessing based on the pointer size if the
implementation provides it directly.
The spec is actually testing a behaviour stemming from NUM2INT(), and
since `sizeof(long)>=sizeof(int)`, `min_long-1` always makes NUM2INT()
raise `RangeError`.
@XrXr XrXr requested a review from eregon July 24, 2024 14:52
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this is great!

@eregon
Copy link
Member

eregon commented Jul 24, 2024

FWIW I think one reason ruby/spec or mspec didn't use much RbConfig::SIZEOF["type"] is it didn't exist in older versions and it's a C extension, which can be annoying e.g. when adding support for a new platform for TruffleRuby and it boots but loading C extensions doesn't work yet; or e.g. when working on a new Ruby impl which likely does not have RbConfig::SIZEOF yet. But since it's all guarded with LoadError that should work fine or be easy to work around.

@eregon eregon merged commit d08e551 into ruby:master Jul 24, 2024
110 checks passed
@XrXr XrXr deleted the fix-integer-size-method branch July 24, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants