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 json.hpp compilation issue with other typedefs with same name (Issue #1642) #1643

Merged
merged 3 commits into from
Jul 1, 2019

Conversation

kevinlul
Copy link
Contributor

@kevinlul kevinlul commented Jun 16, 2019

While in our case, only the string case was affected, to be safe and allow the library to work with other unforeseen cases, all of the cases have been wrapped with parentheses.

Thank you @DyXel and @edo9300

Fixes #1642


Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

While in our case, only the string case was affected, to be safe and allow the library to work with other unforeseen cases, all of the cases have been wrapped with parentheses.

Thank you @DyXel and @edo9300
@coveralls
Copy link

coveralls commented Jun 17, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling e616d09 on kevinlul:develop into 4fc98e0 on nlohmann:develop.

@kevinlul kevinlul changed the title Fix #1642 Fix json.hpp compilation issue with other typedefs with same name (Issue #1642) Jun 17, 2019
Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Can we also add a regression test?

@kevinlul
Copy link
Contributor Author

Do you mean adding a test case to tests/ for this behaviour?

@nlohmann
Copy link
Owner

Yes. Best to unit-regression.cpp if this is possible.

@nlohmann
Copy link
Owner

@kevinlul Let me know if you need help on the test case.

@kevinlul
Copy link
Contributor Author

It seems like boolean is already a definition somewhere under VS2015, including Windows headers?

@nlohmann
Copy link
Owner

Indeed - the error only occurs in the configuration where we include windows.h to detect exactly these kind of issues... I think it would be best not to mention boolean in the regression test.

@nlohmann nlohmann self-assigned this Jul 1, 2019
@nlohmann nlohmann added this to the Release 3.6.2 milestone Jul 1, 2019
@nlohmann nlohmann merged commit 9289a23 into nlohmann:develop Jul 1, 2019
@nlohmann
Copy link
Owner

nlohmann commented Jul 1, 2019

Thanks!

@nlohmann nlohmann removed the platform: visual studio related to MSVC label Jul 1, 2019
@nlohmann
Copy link
Owner

nlohmann commented Jul 1, 2019


🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

  • ✨ New Feature
  • 🐛 Bug Fix
  • ⚡️ Improvement
  • 🔨 Further Change
  • 🔥 Deprecated function

Description


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.

json.hpp compilation issue with other typedefs with same name
3 participants