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

Restore disabled check for #3070 (except on MSVC) #3421

Merged
merged 1 commit into from
Apr 6, 2022

Conversation

falbrechtskirchinger
Copy link
Contributor

Restore the previously disabled check for regression #3070 on all compilers but MSVC.

To summarize the issue:
Given namespace fs = std::filesystem.
On MSVC attempting to construct an fs::path from json results in an ambiguous overload resolution because fs::path can be constructed from both a std::string as well as another fs::path. To the best of my knowledge there is no way to fix an ambiguous overload situation involving a type we do not control and with json implicitly converting to both std::string and fs::path.

Re-enabling the check where it compiles and keeping it disabled for MSVC is the best we can do.

Closes #3377 and #3382.

Restore the previously disabled check for regression nlohmann#3070 on all
compilers but MSVC.

To summarize the issue:
Given namespace fs = std::filesystem.
On MSVC attempting to construct an fs::path from json results in an
ambiguous overload resolution because fs::path can be constructed from
both a std::string as well as another fs::path.
To the best of my knowledge there is no way to fix an ambiguous overload
situation involving a type we do not control and with json implicitly
converting to both std::string and fs::path.

Re-enabling the check where it compiles and keeping it disabled for MSVC
is the best we can do.

Closes nlohmann#3377 and nlohmann#3382.
@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Apr 5, 2022

Looking at #3229 we may want to disable it for ICPC as well. Should allow the PR to pass CI and be merged.

Edit: I propose to merge this first. I've rebased https://github.com/nlohmann/json/tree/icpc and believe I can tackle ICPC and the doctest version bump (by excluding doctest from clang-tidy) in one swoop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling db1196f on falbrechtskirchinger:resolve-3377 into ab5cecb on nlohmann:develop.

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 Apr 6, 2022
@nlohmann nlohmann added this to the Release 3.10.6 milestone Apr 6, 2022
@nlohmann nlohmann merged commit c2054b9 into nlohmann:develop Apr 6, 2022
@falbrechtskirchinger falbrechtskirchinger deleted the resolve-3377 branch April 6, 2022 06:28
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.

Regression test for #3070 is not being run and fails when enabled
3 participants