Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

[GDC] Do not include padding when hashing floating point types #2356

Merged
merged 1 commit into from
Nov 18, 2018

Conversation

jpf91
Copy link
Contributor

@jpf91 jpf91 commented Nov 18, 2018

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.

ping @ibuclaw

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.
@dlang-bot
Copy link
Contributor

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 verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

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 references

Your 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 locally

If 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"

@thewilsonator
Copy link
Contributor

Looks fine to me. cc resident hashing expert @n8sh

Copy link
Member

@ibuclaw ibuclaw left a 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];
Copy link
Member

@ibuclaw ibuclaw Nov 18, 2018

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.

Copy link
Contributor

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!

@n8sh
Copy link
Member

n8sh commented Nov 18, 2018

LGTM.

@dlang-bot dlang-bot merged commit 2b6673f into dlang:master Nov 18, 2018
jpf91 added a commit to jpf91/GDC that referenced this pull request Nov 24, 2018
by backporting dlang/druntime#2356

From druntime commit 29ce0543cb62229f005b2bc8540416dbccd1130e
jpf91 added a commit to jpf91/GDC that referenced this pull request Nov 28, 2018
by backporting dlang/druntime#2356

From druntime commit 29ce0543cb62229f005b2bc8540416dbccd1130e
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants