Details
- Reviewers
ldionne EricWF Mordante zoecarver curdeius • Quuxplusone - Group Reviewers
Restricted Project - Commits
- rG22052860959c: [libcxx][iterator] adds `std::weakly_incrementable` and `std::incrementable`
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This LGTM sans the small nits.
libcxx/include/iterator | ||
---|---|---|
2567 | I assume this wasn't intentional. | |
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.inc/incrementable.compile.pass.cpp | ||
202 | (Non-blocking.) Does it really make sense to test vector and optional, particularly? It seems pretty obvious that these aren't incrementable. | |
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.winc/weakly_incrementable.compile.pass.cpp | ||
53 | You test this exact same thing in the other file. Let's only test it in one place (this file makes more sense to me than the other one). | |
214 | Same comment. |
This LGTM generally speaking, but I'd like to understand the story for signed-integer-class types.
libcxx/include/__iterator/concepts.h | ||
---|---|---|
62 ↗ | (On Diff #339276) | From http://eel.is/c++draft/iterator.concept.winc#11:
Here, we detect if it models signed_integral, but not if it's a signed-integer-class type. Reading the description of what a signed-integer-class type is (starting at http://eel.is/c++draft/iterator.concept.winc#2), it's not clear to me how to even check for that. Do you know what's the intent of the standard here? |
libcxx/include/iterator | ||
64 | I think those comments could be dropped since they don't add much IMO. Feel free to leave in this patch and we'll address that as a NFC on the whole file later. |
libcxx/include/__iterator/concepts.h | ||
---|---|---|
62 ↗ | (On Diff #339276) | http://wg21.link/p1522 conveys the full discussion, but the tl;dr is that LWG were concerned about the distances causing overflow between two iterators in the same range (e.g. iota_view). Implementers (and only implementers) are free to provide integer class types with an range larger than long long. We don't do this (our __int128_t is an extended integral type), so we can get away with this definition (which I learnt in Belfast is allowed, cc @tcanens). Implementing __is_signed_integer_like to account for class types is computationally complex and really error-prone to get right (similar to the old std::boolean concept), so I'd prefer not to do it without first having a class type that we care about. |
libcxx/include/iterator | ||
64 | Let's leave it for an NFC, for consistency's sake? (i.e. I plan to add them to all future iterator concept patches, and then we can delete them in one fell swoop.) |
I like that we're splitting the conformance tests into their own files like iterator_concept_conformance.compile.pass.cpp.
Ship it once you fix the CI, which fails in a few places (e.g. https://buildkite.com/llvm-project/libcxx-ci/builds/2701#e2424e5f-d53f-4f92-a971-328527c80682).
libcxx/include/__iterator/concepts.h | ||
---|---|---|
62 ↗ | (On Diff #339276) | Thanks a lot for the reference! So it does look like we indeed don't need to do anything here unless/until we decide to add a user-defined integer-class type. |
I think those comments could be dropped since they don't add much IMO. Feel free to leave in this patch and we'll address that as a NFC on the whole file later.