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

optional: use #if instead of #ifndef, fixes #3859 #4526

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

theodelrieu
Copy link
Contributor

Hi, it's been a while.

Just connected to Github and saw the optional-related issue, tests pass with -DJSON_ImplicitConversions=OFF.

Copy link

github-actions bot commented Dec 2, 2024

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @theodelrieu
Please read and follow the Contribution Guidelines.

@nlohmann
Copy link
Owner

nlohmann commented Dec 2, 2024

Welcome back! You need Astyle 3.1 to amalgamate.

@github-actions github-actions bot added S and removed L labels Dec 2, 2024
Copy link

github-actions bot commented Dec 2, 2024

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @theodelrieu
Please read and follow the Contribution Guidelines.

@theodelrieu
Copy link
Contributor Author

Making 3.1 work on macOS is too much of a pain, I've edited the amalgamated file by hand 🤡

@nlohmann
Copy link
Owner

nlohmann commented Dec 2, 2024

I downloaded it and built it myself. It's indeed a pain I hope to fix soon.

@theodelrieu
Copy link
Contributor Author

I downloaded it and built it myself.

So did I, but running it didn't do anything, which is why I tried to edit by hand, which doesn't seem to work either...

@coveralls
Copy link

coveralls commented Dec 2, 2024

Coverage Status

coverage: 99.65% (+0.001%) from 99.649%
when pulling a73a715 on theodelrieu:develop
into a006a7a on nlohmann:develop.

@nlohmann
Copy link
Owner

nlohmann commented Dec 2, 2024

Oh, I see we are not executing the test suite with disabled implicit conversions. I will take care of this.

We actually do check with in job ci_test_noimplicitconversions.

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
Copy link
Owner

nlohmann commented Dec 2, 2024

Will check #3859 after merging.

@nlohmann
Copy link
Owner

nlohmann commented Dec 2, 2024

@theodelrieu
Copy link
Contributor Author

Seems that JSON_HAS_CPP_17 is defined, but MSVC doesn't set /std:c++17?
Shouldn't that flag be set when invoking cmake?

I don't have a Windows setup, so it'd be nice if a kind soul could take the burden.

@nlohmann
Copy link
Owner

In .github/external_ci/appveyor.yml, can you try changing

    - APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
      configuration: Release
      platform: x86
      CXX_FLAGS: "/W4 /WX"
      CMAKE_OPTIONS: "-DJSON_ImplicitConversions=OFF"
      GENERATOR: Visual Studio 16 2019

to

    - APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
      configuration: Release
      platform: x86
      CXX_FLAGS: "/permissive- /std:c++latest /utf-8 /W4 /WX"
      CMAKE_OPTIONS: "-DJSON_ImplicitConversions=OFF"
      GENERATOR: Visual Studio 16 2019

The AppVeyor build rules predate the ci.cmake scripts which should take care of setting all the flags properly.

@github-actions github-actions bot added the CI label Dec 12, 2024
@nlohmann
Copy link
Owner

Uff, AppVeyor is still failing... I will see if I can update the jobs there to the CI.cmake file. If I can't do this tomorrow, it will take some more time.

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.

3 participants