-
-
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
Make iterator_proxy_value a forward_iterator (#4371) #4372
base: develop
Are you sure you want to change the base?
Conversation
Ahh - I see from the build logs that these jobs use astyle 3.1; when I ran Re-running with the matching version of astyle produces much saner results :) Though it still does remove a bunch of blank lines in the single-include headers. |
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated. @captaincrutches |
Okay, I reset the single-include files entirely and tried again, and now we've got a diff that actually looks correct. Sorry for the noise! I'm happy to rebase and clean up these extra bad commits if that would be preferable. |
Clang-tidy failures look unrelated to my change - looks like #4311 already exists trying to fix them. I'm not sure what to make of the xcode 12.4 build - seems like something stalled out and hit a timeout, but I can't tell what... the rest of the build looks like it was going fine up until it stopped in its tracks. |
I'll check the Clang-Tidy warnings. |
See #4371 for all the details - but long story short, there seems to be no reason not to expose
iterator_proxy_value
as aforward_iterator
instead of just aninput_iterator
. This allows more use cases whenitems()
is fed into C++20std::views
.If there is a reason not to do so, please let me know - but the
iterator_category
on this type currently seems to long predate the more recent change to fix it for C++20 ranges initially. As well, the actual json iterator which the proxy wraps is also a forward iterator, so this keeps them on par with each other in terms of capabilities.Another option is to remove this
iterator_category
typedef entirely, and letiterator_traits
deduce it - since the type otherwise satisfiesForwardIterator
, it should (and does, in my motivating case) do the right thing. But since the other required typedefs are here, might as well keep it but change it IMO.I'm not sure what, if any, tests can/should be updated/added in this case - I'm open to suggestions, though it seems like all the relevant
static_assert
s and such should still apply.Pull request checklist
Read the Contribution Guidelines for detailed information.
include/nlohmann
directory, runmake amalgamate
to create the single-header filessingle_include/nlohmann/json.hpp
andsingle_include/nlohmann/json_fwd.hpp
. The whole process is described here.Note on amalgamation: I did run
make amalgamate
, and that touched a bunch of lines unrelated to my change. The actual change I've made is toiterator_proxy.hpp
, and hopefully hiding whitespace in the diff will narrow that down. If you'd rather I revert the other amalgamate changes to keep this diff confined to the 2 (well, 4, including generated code) lines I changed, I'm more than happy to do so - but I figured I'd err on the side of caution w.r.t. any checks that run in CI.