Details
- Reviewers
EricWF zoecarver Mordante cjdb - Group Reviewers
Restricted Project - Commits
- rG97e383aa061b: [libc++] Add std::ranges::iter_move and std::iter_rvalue_reference_t
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/iterator | ||
---|---|---|
2476 | I am slightly concerned about the number of overloads that are introduced and the instantiation cost, which is quite high on clang right now due to lack of memoization of concepts. I would rather put everything in a single operator() and then use if constexpr to return the right thing. That would enable us to simplify the concept definitions, as we do not need to exclude the previous concepts |
libcxx/include/iterator | ||
---|---|---|
2476 |
How long ago did Clang disable memoisation?
I'm aware of this optimisation from GCC Concepts TS days, but I'd like to see evidence that Clang will get a significant performance boost from this too. |
libcxx/include/iterator | ||
---|---|---|
2439 | This doesn't match the standard. The "poison pill" that doesn't poison anything is there solely to clarify that we only do ADL. | |
2442 | This is missing the "E has class or enumeration type" part of the spec. In particular, we don't want to do ADL for pointers. | |
2443 | This is not in the spec? | |
2445 | nor is the __can_reference here. | |
2460 | __dereferenceable is stronger than the spec, which only requires *E to be well-formed, not that it returns something referenceable. | |
2476 | Conditional noexcept is required for conformance since the wording uses expression-equivalent. |
fixes iter_move per @tcanens' feedback
moves __class_or_enum out of __swap scope since it's useful elsewhere
libcxx/include/iterator | ||
---|---|---|
2476 | Note that we do have conditional noexcept with MSVC and I believe libstdc++ You have have a look at the _Choice machinery @CaseyCarter build. It is super helpful | |
2498 | What is it with those strange ! |
libcxx/include/iterator | ||
---|---|---|
2476 | I think it would be best to benchmark before adopting a more complex solution in the name of optimisation. |
libcxx/include/iterator | ||
---|---|---|
2498 | They indicate there are non-visible code points in the text (e.g. zero-width joiners). I think it's a part of the LaTeX translation process for the standard. I'm committed to hunting all of these down, so thanks for the heads up. |
Just some super small things. But LGTM!
libcxx/include/concepts | ||
---|---|---|
452 | Can't this just be = is_lvalue_reference_v<_Tp>;? | |
455 | Same as above but for rvalue reference. Then we could get rid of __core_reference. | |
457 | Nit: want to use your new macro here ;) ? | |
libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp | ||
25 | Given the discussion about unions and enums, want to add a super simple type traits test for unqualified lookup on both an enum and union? | |
96 | I'm OK with this implementation. You might consider a static global to track this stuff in the future, though. Especially if you need more than a boolean. As I said, though, I'm OK with this implementation here. | |
139 | I think this is a really good test! | |
187 | No need to call assert here. |
@miscco's suggestion has been gnawing at me for a little while, so I checked it out with someone extremely knowledgeable about both core C++ and Clang. They encouraged benchmarking to be sure, but said they expect if constexpr to be faster than an overload set. They also said it isn't unlikely that the speed boost would be significant, and that there's a good chance the memory usage would be lower too.
With all that in mind, I think the added complexity is justified.
libcxx/include/concepts | ||
---|---|---|
455 | Subsumption means that we need to have all three if we plan to use both __lvalue_reference and __rvalue_reference (see integral, signed_integral, and unsigned_integral). I can technically get away with just introducing __lvalue_reference, but I'd rather hedge that __rvalue_reference will be necessary down the line and get it right on the first go. | |
libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp | ||
187 | Don't we want to check the run-time value as well? |
With all that in mind, I think the added complexity is justified.
I actually think this if-constexpr implementation isn't too bad. While it does add some complexity, it is still readable, and much better than I expected, tbh. But please add a comment above the operator() explaining why we're using this implementation.
But please add a comment above the operator() explaining why we're using this implementation.
I think the explanation should go elsewhere, with a small comment directing people to that. This way we don't need to repeat the explanation for every CPO and niebloid in the library.
I'm thinking at the bottom of the doc you created; WDYT?
I disagree with the choice to use if constexpr over overload resolution for compile-time performance reasons. I think we should continue to strive to keep the code simple. No offense, but the latest revision of this patch with if constexpr is *significantly* more complicated than a naive implementation like this (which is similar to what Chris suggested originally, but a bit simpler IMO):
namespace ranges::__iter_move { void iter_move(); template<class _Ip> concept __unqualified_iter_move = requires(_Ip&& __i) { iter_move(_VSTD::forward<_Ip>(__i)); }; // [iterator.cust.move]/1 // The name ranges::iter_move denotes a customization point object. // The expression ranges::iter_move(E) for a subexpression E is // expression-equivalent to: struct __fn { // [iterator.cust.move]/1.1 // iter_move(E), if E has class or enumeration type and iter_move(E) is a // well-formed expression when treated as an unevaluated operand, [...] template<class _Ip> requires __class_or_enum<remove_cvref_t<_Ip>> && __unqualified_iter_move<_Ip> constexpr decltype(auto) operator()(_Ip&& __i) const noexcept(iter_move(_VSTD::forward<_Ip>(__i))) { return iter_move(_VSTD::forward<_Ip>(__i)); } // [iterator.cust.move]/1.2 // Otherwise, if the expression *E is well-formed: // 1.2.1 if *E is an lvalue, std::move(*E); // 1.2.2 otherwise, *E. template<class _Ip> requires !(__class_or_enum<remove_cvref_t<_Ip>> && __unqualified_iter_move<_Ip>) && requires(_Ip&& __i) { *_VSTD::forward<_Ip>(__i); } constexpr decltype(auto) operator()(_Ip&& __i) const noexcept(*_VSTD::forward<_Ip>(__i)) { if constexpr (is_lvalue_reference_v<decltype(*_VSTD::forward<_Ip>(__i))>) { return _VSTD::move(*_VSTD::forward<_Ip>(__i)); } else { return *_VSTD::forward<_Ip>(__i); } } // [iterator.cust.move]/1.3 // Otherwise, ranges::iter_move(E) is ill-formed. }; } // namespace ranges::__iter_move
Bending the implementation backwards to improve compilation times when the problem is actually at the language level (overload resolution being so expensive) and at the specification level (they could have decided to go for much simpler rules with a lot less expensive SFINAE) is not the right way to go. If we want to improve the compile-times of Ranges in libc++, I'm all for finding ways to do that, but not by making our implementation significantly more complex and working around the language. Also, complexifying the implementation is not only a matter of "oh but it's hard to read". It's primarily about shipping a correct implementation, which I think is more important than shipping one that compiles fast.
Don't get me wrong, I spent like 3 years of my life trying to improve the compilation times in the C++ ecosystem by doing things like Boost.Hana and http://metaben.ch. I'm very sensitive to that problem. I just think that throwing if constexpr at the problem here is too little too late.
libcxx/include/concepts | ||
---|---|---|
250–253 | I really think the remove_cvref_t doesn't belong here. It belongs to the callers of this concept. Even though this concept is just a private helper, I think we should strive to make it meaningful on its own. | |
libcxx/include/iterator | ||
59 | Missing // Since C++20 | |
2435 | Would it make sense to add this in __ranges/iter_move.h? | |
2457 | This is redundant since it's already included in the check for !__lvalue_iter_move<_Ip>. | |
2463 | Not used anywhere. | |
2513 | More strange !s | |
libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp | ||
2 | Can you please add a test with an iterator that can't be iter_moved to confirm that ranges::iter_move(E) SFINAEs away? With the if constexpr implementation, I don't think that's the case. |
libcxx/include/concepts | ||
---|---|---|
250–253 | Re this and your above comment pinging @tcanens: I believe the intent of the spec is that "class [including struct and union] or enumeration types" are exactly the set of types which are allowed to... (in the case of iter_move) ...have their own overloaded operator*. (Enums can have a free-function operator* found by ADL. Primitive types cannot have an overloaded operator* at all.) (in the case of swap) ...have their own free-function swap that might reasonably do something different from the normal swap. (Pointer types can have associated namespaces which end up producing associated swap functions, but those swap functions aren't semantically allowed to do anything except swap the pointers. If this weren't true, our strategy of specializing std::sort for pointer types would be non-conforming. https://godbolt.org/z/G8d6GKbME ) It might be reasonable to rename this concept/trait to something like __causes_adl — but I think __class_or_enum is reasonable too, in that it matches the Standardese term-of-art for "the kind of thing that suffers from ADL." I also think, under this rationale, it makes perfect sense to want to move the cvref-dequalification into the concept/trait itself, because the cvref-qualification of a type doesn't have any effect on whether it suffers from ADL. |
Regarding if constexpr-with-a-single-overload vs normally written code: I just spoke to Chris offline and I'm going to try and chat with Casey about his previous experience with that. I don't want us to make our implementation super complex unless we know for sure it will be a serious problem, and that using if constexpr is going to make a significant difference. I think it is also possible to try to be conscious about reducing the number of overloads and the complexity of the implementation without necessarily going all the way and enforcing a single overload all the time. Let's put this on standby for a few hours until I've gotten more information.
After talking to Chris, Casey and Richard offline, I believe the following approach is the most sensible:
- Write things as simply as possible, without purposefully pessimizing compile-times upfront
- Try to be cognizant of the fact that overloads are not cheap, especially in the presence of complicated SFINAE (which is pretty much all the time with concepts)
- Go back with more field experience and optimize the compile-time of the implementation based on what we see is truly slowing things down
That way, we're not going to cargo-cult one way of doing things before we have actual data to justify an approach. We'll also be able to optimize just the things that matter in practice, which is far from clear at this early stage.
Based on offline discussion with Chris, I'm going to commandeer this now to go back to the pre constexpr if approach.
libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp | ||
---|---|---|
187 | Yes, I think we do. I'm leaving the assert in place. |
libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp | ||
---|---|---|
187 | Why do we want to check the runtime value? The end of check_iter_move is return true; we're not "checking" anything. |
libcxx/include/__iterator/iter_move.h | ||
---|---|---|
59 ↗ | (On Diff #338215) | This noexcept specifier doesn't account for a throwing move. |
libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp | ||
187 | Sure, but the contents of check_iter_move need to be evaluated at both compile-time and run-time. |
libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp | ||
---|---|---|
187 | I agree. But why can't you just call check_iter_move normally? Why do you need the assert? |
libcxx/include/__iterator/iter_move.h | ||
---|---|---|
59 ↗ | (On Diff #338215) | "move doesn't move," and this function returns decltype(auto), so I think we're okay here. This function either returns an rvalue reference in the if, or URVOs a prvalue in the else. |
libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp | ||
187 | +1 @zoecarver |
libcxx/include/__iterator/iter_move.h | ||
---|---|---|
59 ↗ | (On Diff #338215) | @rsmith is the one who pointed it out to me, so I'm not so sure. Granted, the tests cover seem to be passing, but a looking over them again makes me realise I only tested noexcept specifiers in one direction. We'll need to plug those holes at the very least. |
libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp | ||
187 | Oh, you're talking about literally removing assert, not what's inside it. I'm okay with that! |
libcxx/include/concepts | ||
---|---|---|
252 | More or less what Arthur said below. There is no reason to treat unions differently from other classes in the contexts at issue. |
libcxx/include/__iterator/iter_move.h | ||
---|---|---|
59 ↗ | (On Diff #338215) |
That was my reasoning when writing that.
Sorry, I'm not quite following, what do you mean by testing noexcept specifiers only in one direction? I'll add the test coverage, but I'd like to understand what's missing. |
libcxx/include/__iterator/iter_move.h | ||
---|---|---|
59 ↗ | (On Diff #338215) | The tests in this commit exclusively assume noexcept(std::ranges::iter_move(lvalue)) == false and noexcept(std::ranges::iter_move(rvalue)) == true. That was an oversight in my design testing (and what I get for not knowing how to do TDD properly in 2019). |
libcxx/include/__iterator/iter_move.h | ||
---|---|---|
45 ↗ | (On Diff #338215) | Please make sure all overloads are [[nodiscard]] before submitting too. |
🚢
libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.nodiscard.verify.cpp | ||
---|---|---|
35 ↗ | (On Diff #338994) | This is nice :-) |
A few comments, sorry. Nothing blocking, though, so feel free to commit.
libcxx/include/__iterator/iter_move.h | ||
---|---|---|
15 ↗ | (On Diff #338994) | Why are these (or at least why does it appear that these) are both importing __class_or_enum? (I think only the latter should have the comment.) |
49 ↗ | (On Diff #338994) | Non blocking nit: _LIBCPP_NOEXCEPT_RETURN? |
libcxx/include/concepts | ||
446–457 | Non blocking, nit: _LIBCPP_HAS_NO_RANGES | |
libcxx/include/iterator | ||
453 | I'd rather have this be such that we can import any libc++ header and it will guard against the correct stdlib version, etc. internally. This is how we do it for other "versioning" features, I think we should do it with this, too. In other words, I'd rather have the _LIBCPP_HAS_NO_RANGES check inside of iter_move.h. | |
libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp | ||
24 | This header is only used in one place, right? At least, I'd like to see it in this directory and not the parent one. But I don't see any reason it can't just be inlined into this file, if nothing else is using it. | |
187 | Heh, sorry I wan't clear. Yes, that's what I mean :) |
libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp | ||
---|---|---|
24 | The intention is for it to be shared with iter_swap tests. |
libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp | ||
---|---|---|
24 | Fair enough. In that case, disregard my comment. |
libcxx/include/iterator | ||
---|---|---|
453 | +10. This is already causing me issues downstream where I only import headers that expose the necessary names. |
libcxx/include/__iterator/iter_move.h | ||
---|---|---|
15 ↗ | (On Diff #338994) | It was a typo, fixed. |
49 ↗ | (On Diff #338994) | I'm really not a big fan of that macro, and it's only defined locally when defining __apply_tuple_impl. Not doing. |
libcxx/include/concepts | ||
446–457 | Actually, this line isn't touched anymore after rebase, so not changing anything. |
Why are we checking for union here? Is it because the spec says has class or enumeration type, and a union is technically a class type even though is_class returns false for it? @tcanens Do you know what the intent of the spec is in that regard?