Skip to content

Fix conversion to std::optional #4742

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
Merged
merged 7 commits into from
Apr 15, 2025
Merged

Fix conversion to std::optional #4742

merged 7 commits into from
Apr 15, 2025
+55 −14

Conversation

nlohmann
Copy link
Owner

@nlohmann nlohmann commented Apr 14, 2025 •
edited
Loading

This MR fixes the conversion from JSON to std::optional by making it independent of JSON_USE_IMPLICIT_CONVERSIONS.

Fixes #4740

@nlohmann nlohmann mentioned this pull request Apr 14, 2025
Closed
2 tasks
@nlohmann nlohmann linked an issue Apr 14, 2025 that may be closed by this pull request
Build issue with std::optional #4740
Closed
2 tasks
@coveralls
Copy link

coveralls commented Apr 14, 2025 •
edited
Loading

Coverage Status

coverage: 99.188% (+0.002%) from 99.186%
when pulling 6c03aaf on issue4740
into 4cca3b9 on develop.

@nlohmann nlohmann marked this pull request as ready for review April 14, 2025 15:45
@nlohmann nlohmann added the review needed It would be great if someone could review the proposed changes. label Apr 14, 2025
@nlohmann nlohmann added this to the Release 3.12.1 milestone Apr 14, 2025
@nlohmann nlohmann changed the title Add regression test for conversion from/to std::optional Fix conversion to std::optional Apr 14, 2025
vtstmn
vtstmn suggested changes Apr 14, 2025
View reviewed changes
Copy link

@vtstmn vtstmn left a comment

Choose a reason for hiding this comment

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

OK with code modification and unit test.
Shouldn't the from_json() function for std:optional be inlined like other from_json() functions?

@gregmarr
Copy link
Contributor

Shouldn't the from_json() function for std:optional be inlined like other from_json() functions?

No, that's not necessary. This is a template function that is not a full specialization, so the inline modifier is not required. It is only required when fully specialized.

@nlohmann
Copy link
Owner Author

No, that's not necessary. This is a template function that is not a full specialization, so the inline modifier is not required. It is only required when fully specialized.

Would you propose reverting this?

@gregmarr
Copy link
Contributor

Would you propose reverting this?

It doesn't hurt, but it is redundant. You never know when they may add a clang tidy check for it. Since the PR hasn't been merged yet, it might be worth it.

@nlohmann
⏪ remove inline
6c03aaf 
Signed-off-by: Niels Lohmann <[email protected]>
@nlohmann nlohmann requested a review from vtstmn April 15, 2025 12:09
vtstmn
vtstmn reviewed Apr 15, 2025
View reviewed changes
Copy link

@vtstmn vtstmn left a comment

Choose a reason for hiding this comment

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

OK with all modifications

@nlohmann nlohmann merged commit 96c1b52 into develop Apr 15, 2025
134 checks passed
@nlohmann nlohmann deleted the issue4740 branch April 15, 2025 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtstmn vtstmn vtstmn requested changes

Assignees
No one assigned
Labels
M review needed It would be great if someone could review the proposed changes. tests
Projects
None yet
Milestone
Release 3.12.1
Development

Successfully merging this pull request may close these issues.

Build issue with std::optional
4 participants
@nlohmann @coveralls @gregmarr @vtstmn