Skip to content

Fix expansion of __VA_ARGS__ macro argument in MSVC #45

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

Merged
merged 7 commits into from
Feb 25, 2021
Merged

Fix expansion of __VA_ARGS__ macro argument in MSVC #45

merged 7 commits into from
Feb 25, 2021

Conversation

samangh
Copy link
Contributor

@samangh samangh commented Feb 18, 2021

This merge request fixes issues with variadic macro argument expansion in MSVC. The issue is that when passing __VA_ARGS__ as an argument to another macro, MSVC expands __VA_ARGS__ after passing, whilst gcc expands it before.

Issue #31 can be closed.

@samangh
Copy link
Contributor Author

samangh commented Feb 18, 2021

Note: you still need to reduce the number of macro arguments to < 127 for this to work in MSVC. I haven't changed that as @quicknir seems to have settled on using 258 as the default.

@quicknir: how do you feel about reducing the default number of macro argyments to 125, so that the appveyor CI tests get passed.

@samangh
Copy link
Contributor Author

samangh commented Feb 18, 2021

@quicknir: I noticed that in the 'dev' branch you are are already looking at reducing the max number of iterations, so I've also done that in this pull request. Of course, feel free to ignore.

Although this merge request works find on my simple use scenario using C++17, I see that it's still failing the C++14 appveyor tests. Looks like some further debugging will be required ... .

@quicknir
Copy link
Owner

@samangh the reduction in iterations is only to support MSVC, so if that is not necessary to support MSVC, it's fine to leave it at 256. This looks quite promising; even if there are still some issues with MSVC, if this doesn't break Linux (which it seems unlikely to do), I can still merge what we have, as it seems a step in the right direction.

Out of curiosity, do you know whether it's MSVC or gcc that's compliant in terms of the macro expansion order? IIRC variadic macros were officially standardized in C++11 so there should be some wording on this.

@samangh
Copy link
Contributor Author

samangh commented Feb 18, 2021

@quicknir: Yeah, unfortunately the 125 max iterations is a hard compiler limit in MSVC.

I'm not sure about which of the two compilers is more compliant. I suspect that the expansion order is not specified clearly in the standard (although I haven't checked), see the replies to this stackoverflow question for instance.

@samangh
Copy link
Contributor Author

samangh commented Feb 18, 2021

Looks like I managed to fix all the issues, so this can be merged.

I also made test/CMakeLists.txt more platform agnostic. We now only check with Visual Studio 2019, as only VS2019 is installed in the appveyor build image.

@samangh
Copy link
Contributor Author

samangh commented Feb 25, 2021

@quicknir: I was wondering if you've had any chance to look at this? Much appreciated.

@quicknir
Copy link
Owner

Hey sorry! I lost track of this, glad you pinged me. All looks good, merging. Thanks very much for your help.

@quicknir quicknir merged commit 703b7f4 into quicknir:master Feb 25, 2021
@samangh
Copy link
Contributor Author

samangh commented Feb 25, 2021

@quicknir: No problem. Glad to see that you were happy with the changes :-). If you get a chance, could you also look at merge request #27?

Thanks for your work on wise_enum!

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.

2 participants