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

improve compatibility for _MSC_VER #514

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GitMensch
Copy link
Contributor

copy from lexxmark/winflexbison@2a7586a by @yjh-styx - partial work on #367

@Explorer09
Copy link
Contributor

I think this missed a check that MSVC compiles code only for little-endian machines. Otherwise the semantic would be wrong.
htonl and htons should be no-op when compiled for big-endian and become byte swap functions for little-endian.

@Explorer09
Copy link
Contributor

Update: I think it would be good enough to cite this in the code comments
https://learn.microsoft.com/en-us/cpp/standard-library/bit-enum?view=msvc-170
"All native scalar types are little-endian for the platforms that Microsoft Visual C++ targets"

@GitMensch
Copy link
Contributor Author

So you suggest to make those macros a no-op, right?

@Explorer09
Copy link
Contributor

Explorer09 commented Mar 1, 2023

@GitMensch No. The byte order does need to be swapped for little endian. What I meant is add a comment that MSVC only supports little endian platforms and you have the macros defined the way you did them.

#ifdef _MSC_VER
/* MSVC only supports little endian. 
(https://learn.microsoft.com/en-us/cpp/standard-library/bit-enum?view=msvc-170) */
#include <stdlib.h>
#define htonl(n) _byteswap_ulong(n)
#define htons(n) _byteswap_ushort(n)
#endif

@GitMensch
Copy link
Contributor Author

GitMensch commented Mar 1, 2023

I see, thanks for the clarification.
This PR is almost 14 months old, but if someone with pull access wanted to have this change, I'd be happy to do it.

@Croydon
Copy link

Croydon commented Mar 3, 2023

@Explorer09 you can use GitHub's code suggestion feature, then @GitMensch can apply the changes with a few clicks and you get credits :)

@Explorer09
Copy link
Contributor

you can use GitHub's code suggestion feature, then @GitMensch can apply the changes with a few clicks and you get credits :)

I was expecting that @GitMensch adjust the formatting of my suggested change instead of applying the changes as is.
Note that _byteswap_ulong and _byteswap_ushort depend on <stdlib.h> in MSVC's build environment, so it's not just a simple function (macro) replacement.

@westes
Copy link
Owner

westes commented Apr 6, 2024

If someone wants to implement @xplorer09's suggestion (with whatever else MS stuff needs to be done inside the ifdef), I would merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants