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

fix issue 19208 - swapEndian doesn't support floating point types #6720

Closed
wants to merge 2 commits into from
Closed

fix issue 19208 - swapEndian doesn't support floating point types #6720

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 26, 2018

  • Refactor existing union with a more elegant static expression.
  • Set the function parameter const because parameter is always a value and this can reduce how many instances are generated.

@ghost ghost requested a review from andralex as a code owner September 26, 2018 12:46
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @bbasile! 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

Auto-close Bugzilla Severity Description
19208 normal std.bitmanip.swapEndian doesn't support floating point types

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 + phobos#6720"

std/bitmanip.d Outdated
T swapEndian(T)(T val) @safe pure nothrow @nogc
if (isIntegral!T || isSomeChar!T || isBoolean!T)
T swapEndian(T)(const T val) @safe pure nothrow @nogc
if (canSwapEndianness!T)
Copy link
Member

@n8sh n8sh Oct 1, 2018

Choose a reason for hiding this comment

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

It appears that return type T may not work properly for floats. From the documentation of bigEndianToNative:

/++
...
    Taking a `ubyte[n]` helps prevent accidentally using a swapped value
    as a regular one (and in the case of floating point values, it's necessary,
    because the FPU will mess up any swapped floating point values. So, you
    can't actually have swapped floating point values as floating point values).
+/

To really test if this PR works, instead of testing swapEndian(0.5f).swapEndian() == 0.5f perhaps check swapEndian(0x1.00007ep+0f) == 0x1.01007ep-1f. Although it should be asked how the FPU was expected "mess up" swapped values so it can be verified this doesn't happen.

Copy link
Author

Choose a reason for hiding this comment

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

Yes indeed. I did that because it's hard to get two exact representations of floats when swapped without cheating.

Copy link
Author

Choose a reason for hiding this comment

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

swapping a valid float value that leads to NaN is out of topic i think, even if a true topic. The operation might be valid for serialization purpose.

Copy link
Member

Choose a reason for hiding this comment

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

I can't see how it is valid – just use a ubyte buffer on the serialised side.

Copy link
Author

Choose a reason for hiding this comment

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

It depends on the serializer.

Copy link
Member

Choose a reason for hiding this comment

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

If deserializing a messed-up float can result in it getting changed, then this doesn't work for either serialization or deserialization.

Copy link
Author

Choose a reason for hiding this comment

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

Let's stop speaking about serialization, i don't know why i put this off topic point on the table. I'll just change the test, as initially suggested.

Copy link
Member

Choose a reason for hiding this comment

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

What I think we're driving at is - under what circumstance would this function be useful? All it seems like now is a trap that appears useful for a certain purpose but will sometimes give garbage output.

Copy link
Author

Choose a reason for hiding this comment

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

About your first comment @n8sh : I've added an overload for x86 that doesn't pass the parameter in ST(0).

@ghost ghost closed this Oct 10, 2018
@ghost ghost deleted the issue-19208 branch October 10, 2018 11:54
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants