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

Protect macro expansion of commonly defined macros #1337

Closed
SylvainCorlay opened this issue Nov 6, 2018 · 7 comments
Closed

Protect macro expansion of commonly defined macros #1337

SylvainCorlay opened this issue Nov 6, 2018 · 7 comments
Assignees
Labels
platform: visual studio related to MSVC release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@SylvainCorlay
Copy link

SylvainCorlay commented Nov 6, 2018

Wrapping std::min and std::max in parentheses prevents the expansion of windows' min and max macros. I agree these should have never existed in the first place, but that would allow windows users to make use of nlohmann_json without setting NOMINMAX.

Similarly, Python.h defines snprintf as a macro. Wrapping calls in parentheses would also prevent the expansion of this macro.

So the fix would be to replace instances of std::snprintf with (std::snprintf), similarly to what is done with std::max and std::min.

@nlohmann
Copy link
Owner

nlohmann commented Nov 7, 2018

I am not sure what this issue proposes - adding parentheses around min and max? Then point me to a place where the parentheses are missing.

@nlohmann nlohmann added the state: needs more info the author of the issue needs to provide more details label Nov 7, 2018
@SylvainCorlay
Copy link
Author

Sorry if I was not clear. I was suggesting to do the same with snprintf as for min and max, that is to wrap it in parentheses. snprintf is defined as a macro by Python.h (as equal to _snprintf).

I met the issue for std::snprintf, which is used in serializer.hpp and binary_reader.hpp.

@nlohmann
Copy link
Owner

nlohmann commented Nov 7, 2018

Thanks for clarifying! I shall have a look.

@nlohmann nlohmann added platform: visual studio related to MSVC and removed state: needs more info the author of the issue needs to provide more details labels Nov 7, 2018
@nlohmann
Copy link
Owner

nlohmann commented Nov 9, 2018

@SylvainCorlay Could you please check whether https://github.com/nlohmann/json/tree/feature/snprintf fixes the issue?

@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Nov 9, 2018
@SylvainCorlay
Copy link
Author

@nlohmann thanks! I could check and it works perfectly!

@nlohmann nlohmann self-assigned this Nov 10, 2018
@nlohmann nlohmann added this to the Release 3.4.1 milestone Nov 10, 2018
@nlohmann
Copy link
Owner

Thanks for the feedback!

@SylvainCorlay
Copy link
Author

Thanks @nlohmann. And thanks for the great work on this library!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: visual studio related to MSVC release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

2 participants