-
-
Notifications
You must be signed in to change notification settings - Fork 418
[GDC] Do not include padding when hashing floating point types #2356
Conversation
The exact contents of the padding is not specified. Usually it's 0, but some optimizations in GDC sometimes lead to different padding content. For floating point types without padding, the code is unchanged. For real or imaginary types with padding, just ignore the trailing padding. For complex types where the real/imaginary types have padding, we can't hash the value as one contiguous memory block, as there will be a padding 'hole'. Here, hash real and imaginary part individually. Complex types without padding are still hashed as one memory block.
Thanks for your pull request and interest in making D better, @jpf91! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + druntime#2356" |
Looks fine to me. cc resident hashing expert @n8sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me.
else static if (is(T : real) || is(T : ireal)) | ||
{ | ||
// Ignore trailing padding | ||
auto bytes = toUbyte(data)[0 .. floatSize!T]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One consideration is float endianess, but we don't currently expose that in the language with either version condition nor traits (yes, a platform could have one endian for floats, and another for integers).
Not a blocker though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a platform could have one endian for floats, and another for integers
That is the stuff of nightmares!
LGTM. |
by backporting dlang/druntime#2356 From druntime commit 29ce0543cb62229f005b2bc8540416dbccd1130e
by backporting dlang/druntime#2356 From druntime commit 29ce0543cb62229f005b2bc8540416dbccd1130e
The exact contents of the padding is not specified. Usually it's 0, but some optimizations in GDC sometimes lead to different padding content.
individually. Complex types without padding are still hashed as one memory block.
ping @ibuclaw