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

Reimplement value() access functions #3663

Merged

Conversation

falbrechtskirchinger
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger commented Aug 3, 2022

  • Reimplement the value() functions, reducing the number of overloads from 6+2 to 4+2 (the 2 deprecated json_pointer<BasicJsonType> overloads remain) and squashing a few bugs in the process.

  • Update Unit tests to catch reported regressions and test more scenarios.

Fixes #3652.
Fixes #3655.

To Do:

  • Clean up. (Move type traits to type_traits.hpp, etc.)
  • Document changes. (Template parameter order, exposition-only signatures, ...)

@coveralls
Copy link

coveralls commented Aug 3, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling ced42d5 on falbrechtskirchinger:value-ambiguity_issue3652 into 7b6cf59 on nlohmann:develop.

@falbrechtskirchinger
Copy link
Contributor Author

Looks good to me.

@falbrechtskirchinger
Copy link
Contributor Author

I'm trying out a new way of writing/organizing the type traits. I want to clean up type_traits.hpp soon.

using trait_name = std::integral_constant<bool, [trait conditions]>;

This, as it turns out, is a bad idea, as it results in poor compiler diagnostics. From memory: "unmet requirement std::integral_constant<bool, false>"

The type_traits unit test should also be extended. It did safe me from a small mistake with just a singular trait being tested.

Also, I'm only including nlohmann/detail/meta/type_traits.hpp when possible, to improve compilation speed. Maybe we can optimize a few more files (class_lexer, for example).

@nlohmann
Copy link
Owner

nlohmann commented Aug 5, 2022

It would be great if also someone else had a look at this. @gregmarr ?

@falbrechtskirchinger
Copy link
Contributor Author

Fixed the typo. That's it.

* Merges the 'const char *' with the 'ValueType &&' overloads.
* Fixes ambiguities when default value is 0.
* Fixes 'no matching function' error when specifying ValueType template
  parameter.
* Fixes incorrect template parameter order in previous overloads.
Define the macro JSON_TEST_USING_MULTIPLE_HEADERS to 0/1 depending on
JSON_MultipleHeaders.
Copy link
Contributor Author

@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. (Only fixed a typo since last review.)

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 added this to the Release 3.11.2 milestone Aug 7, 2022
@nlohmann nlohmann merged commit 0c7a183 into nlohmann:develop Aug 7, 2022
@falbrechtskirchinger falbrechtskirchinger deleted the value-ambiguity_issue3652 branch August 7, 2022 16:24
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: .value<size_t> is compilation error. Regression: call to member function 'value' is ambiguous
5 participants