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.

Sign up for GitHub

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

Jump to bottom

Fixed latest build error in msvc platform #3630

Merged
merged 3 commits into from
Jul 30, 2022
Merged

Fixed latest build error in msvc platform #3630

merged 3 commits into from
Jul 30, 2022

Conversation

Lioncky
Copy link
Contributor

@Lioncky Lioncky commented Jul 30, 2022

Like #762 warnings in msvc about mix/max macro while windows.h is used in the project

@Lioncky
Fixed latest build error in msvc platform
5450029 
warnings in msvc about mix/max macro while windows.h is used in the project #762
@Lioncky Lioncky requested a review from nlohmann as a code owner July 30, 2022 08:33
@falbrechtskirchinger
Copy link
Contributor

The changes need to be applied to single_include/nlohmann/json.hpp as well. Please either run make amalgamate or apply them manually.

@Lioncky
Copy link
Contributor Author

Lioncky commented Jul 30, 2022

No, single_include/nlohmann/json.hpp doesn't cause the error, msvc with this error can't build. The changes are necessary and not affect other compilers.

@falbrechtskirchinger
Copy link
Contributor

falbrechtskirchinger commented Jul 30, 2022 •
edited
Loading

The header single_include/nlohmann/json.hpp is generated from the headers in include/nlohmann/. You must make these changes for CI to pass.

See https://github.com/nlohmann/json/blob/develop/.github/CONTRIBUTING.md#files-to-change for more information.

While this recommends against manual editing, if you're making these changes through GitHub, that is your only option.

@Lioncky @falbrechtskirchinger
Update single_include/nlohmann/json.hpp
916fa42 
Co-authored-by: Florian Albrechtskirchinger <[email protected]>
falbrechtskirchinger
falbrechtskirchinger approved these changes Jul 30, 2022
View reviewed changes
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

I've added a unit test to catch this issue in the future (#3631).

@falbrechtskirchinger falbrechtskirchinger mentioned this pull request Jul 30, 2022
Merged
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 916fa42 on KsaNL:patch-1 into d1d79b9 on nlohmann:develop.

nlohmann
nlohmann approved these changes Jul 30, 2022
View reviewed changes
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.

Looks good to me.

@nlohmann nlohmann self-assigned this Jul 30, 2022
@nlohmann nlohmann added this to the Release 3.11.0 milestone Jul 30, 2022
@nlohmann nlohmann merged commit fc3e64c into nlohmann:develop Jul 30, 2022
@nlohmann
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@falbrechtskirchinger falbrechtskirchinger falbrechtskirchinger approved these changes

@nlohmann nlohmann nlohmann approved these changes

Assignees

@nlohmann nlohmann

Labels
None yet
Projects
None yet
Milestone
Release 3.11.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Lioncky @falbrechtskirchinger @coveralls @nlohmann