-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Iterator doesn't satisfy std::incrementable because post-increment may change constness #3331
Comments
Those |
Commit f28fc22 introduced const qualifiers on post-(inc-/dec-)rement operators of iterators. These qualifiers prevent the use of basic_json in place of std::ranges::Range, which requires the post-increment operator to be equality-preserving. These changes appear to be the result of compiler suggestions, and no further explanation is discernible from the PR discussion (nlohmann#858). This commit partially reverts f28fc22, removing all added const qualifiers. Fixes nlohmann#3331.
Commit f28fc22 introduced const qualifiers on post-(inc-/dec-)rement operators of iterators. These qualifiers prevent the use of basic_json in place of std::ranges::Range, which requires the post-increment operator to be equality-preserving. These changes appear to be the result of compiler suggestions, and no further explanation is discernible from the PR discussion (nlohmann#858). This commit partially reverts f28fc22, removing all added const qualifiers. Fixes nlohmann#3331.
Commit f28fc22 introduced const qualifiers on post-(inc-/dec-)rement operators of iterators. These qualifiers prevent the use of basic_json in place of std::ranges::range, which requires the post-increment operator to be equality-preserving. These changes appear to be the result of compiler suggestions, and no further explanation is discernible from the PR discussion (nlohmann#858). This commit partially reverts f28fc22, removing all added const qualifiers. Fixes nlohmann#3331.
In light of @gregmarr's findings, I'm going ahead with a PR. Just waiting for the unit tests to finish, before I submit. |
Static analysis on the PR reveals the motivation for the
These guidelines are no longer accessible:
Further investigations lead me to: https://stackoverflow.com/questions/52871026/overloaded-operator-returns-a-non-const-and-clang-tidy-complains lvalue ref-qualifying the operators seems to be the solution. I'll make the change and we'll see if it breaks any tests and silences clang-tidy. |
Commit f28fc22 introduced const qualifiers on post-(inc-/dec-)rement operators of iterators. These qualifiers prevent the use of basic_json in place of std::ranges::range, which requires the post-increment operator to be equality-preserving. These changes appear to be the result of ICC compiler suggestions, and no further explanation is discernible from the PR discussion (nlohmann#858). Further testing revealed, that clang-tidy also suggests adding const to prevent "accidental mutation of a temporary object". As an alternative, this commit partially reverts f28fc22, removing all added const qualifiers from return types and adds lvalue reference qualifiers to the operator member functions instead. Unit tests ensure the operators remain equality-preserving and accidental mutation of temporaries following post-(inc-/dec-)rement is prohibited. Fixes nlohmann#3331.
I'm worried that adding the lvalue qualifiers would be a breaking change. Granted, it is unlikely to be what the user intended, but it's still something that would compile before and won't now. |
I disagree. The behavior should be exactly the same.
Line 3 is (hopefully obviously) unaffected and line 4 fails with virtually the same compiler output (referring to the second use of post-increment):
Note the The Put differently, both qualifiers "break" code that would have compiled before #858, but no additional code should fail to compile after #3332. Both prevent accidental use of UB. Also, isn't that what unit tests are for? 😉 |
I did come up with something. This works before #3332 but not after:
Compiler output after #3332:
Dangling reference prevented. |
Commit f28fc22 introduced const qualifiers on post-(inc-/dec-)rement operators of iterators. These qualifiers prevent the use of basic_json in place of std::ranges::range, which requires the post-increment operator to be equality-preserving. These changes appear to be the result of ICC compiler suggestions, and no further explanation is discernible from the PR discussion (nlohmann#858). Further testing revealed, that clang-tidy also suggests adding const to prevent "accidental mutation of a temporary object". As an alternative, this commit partially reverts f28fc22, removing all added const qualifiers from return types and adds lvalue reference qualifiers to the operator member functions instead. Unit tests ensure the operators remain equality-preserving and accidental mutation of temporaries following post-(inc-/dec-)rement is prohibited. Fixes nlohmann#3331.
What about this?
Just want to make sure that we don't break people unnecessarily. |
I've tested all permutations: Stepping back for a moment. What prompted me filing this issue, was the inability to use I'm fully open to doing more testing if anyone want's to suggest scenarios and maybe add more unit tests accordingly. |
Great, thanks for the detail. |
Commit f28fc22 introduced const qualifiers on post-(inc-/dec-)rement operators of iterators. These qualifiers prevent the use of basic_json in place of std::ranges::range, which requires the post-increment operator to be equality-preserving. These changes appear to be the result of ICC compiler suggestions, and no further explanation is discernible from the PR discussion (#858). Further testing revealed, that clang-tidy also suggests adding const to prevent "accidental mutation of a temporary object". As an alternative, this commit partially reverts f28fc22, removing all added const qualifiers from return types and adds lvalue reference qualifiers to the operator member functions instead. Unit tests ensure the operators remain equality-preserving and accidental mutation of temporaries following post-(inc-/dec-)rement is prohibited. Fixes #3331.
The following code fails to compile:
Here are the relevant excerpts from the compiler output with
-fconcepts-diagnostics-depth=2
:The compiler is telling us that the post-increment operator is adding const to the return type, which violates the requirements of the
std::incrementable
iterator concept.Looking in
json.hpp
we find:Remove
const
from the return type and the introductory code snippet compiles just fine.Someone more familiar with the code base will have to decide whether that is an appropriate fix.
Which version of the library did you use?
develop
branchThe text was updated successfully, but these errors were encountered: