-
-
Notifications
You must be signed in to change notification settings - Fork 700
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
Simplify endian conversion functions in std.bitmanip. #8822
Conversation
Thanks for your pull request, @jmdavis! 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 run digger -- build "master + phobos#8822" |
If I screwed up anything, it would probably be the floating point stuff, but it passes the tests (at least on my machine). |
4707c4a
to
dcb6d97
Compare
I find it somewhat suspicious that I had to change |
I dislike that this PR makes DMD's output much worse without giving any user-visible benefit elsewhere. Prior to this PR DMD does the same thing as LDC.
Post this PR it does not.
I'm only half-joking when I say mentioning this might increase the chance this PR is accepted, but so long as DMD is available for download on the front page of dlang.org I don't think that's the right attitude. This is going to harm anyone who is currently using DMD and std.bitmanip under the assumption—which until this moment has been valid—that it does something reasonable. |
Well, the I guess that I'll have to try it with a string mixin instead so that I can generate a single expression, much as the code is uglier that way. |
Actually, the disassembly that you were showing was for the |
Type punning with a union is not undefined behavior in C. See this footnote from the C11 standard:
|
They're overly complex (e.g. testing for the native endianness of the machine when that's actually completely unnecessary), and they use unions in a manner that is undefined behavior in C/C++ (since they write to one field and then read from another). I don't see any mention of whether that behavior is defined in D in D's spec, but it's probably undefined in D given that it's undefined in C/C++. Either way, it's an overly complex solution. This solution gets rid of all of the endian version checks in std.bitmanip, and it allows the implementations of the endian conversion functions to be the same between CTFE and runtime.
dcb6d97
to
c8d085c
Compare
They're overly complex (e.g. testing for the native endianness of the machine when that's actually completely unnecessary), and they use unions in a manner that is undefined behavior in C/C++ (since they write to one field and then read from another). I don't see any mention of whether that behavior is defined in D in D's spec, but it's probably undefined in D given that it's undefined in C/C++. Either way, it's an overly complex solution.
This solution gets rid of all of the endian version checks in std.bitmanip, and it allows the implementations of the endian conversion functions to be the same between CTFE and runtime.
@atilaneves Assuming that I didn't screw up the implementation somehow, this should be much more to your liking than what's been here. Either way, it results in a lot less code to be screwed up. :)