[libcxx-commits] [PATCH] D99873: [libcxx] adds `std::ranges::iter_move` and `std::iter_rvalue_reference_t`

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

A few comments, sorry. Nothing blocking, though, so feel free to commit.



================
Comment at: libcxx/include/__iterator/iter_move.h:15
+#include <__iterator/concepts.h> // __class_or_enum
+#include <concepts> // __class_or_enum
+#include <type_traits>
----------------
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.)


================
Comment at: libcxx/include/__iterator/iter_move.h:49
+  {
+    return iter_move(_VSTD::forward<_Ip>(__i));
+  }
----------------
Non blocking nit: `_LIBCPP_NOEXCEPT_RETURN`?


================
Comment at: libcxx/include/concepts:445
 
-#endif //_LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)
+#endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)
 
----------------
Non blocking, nit: `_LIBCPP_HAS_NO_RANGES`


================
Comment at: libcxx/include/iterator:439
 
+#if !defined(_LIBCPP_HAS_NO_RANGES)
+#   include <__iterator/iter_move.h>
----------------
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`.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp:23
+
+#include "../unqualified_lookup_wrapper.h"
+
----------------
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. 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp:186
+  static_assert(check_iter_move());
+  assert(check_iter_move());
+
----------------
cjdb wrote:
> Quuxplusone wrote:
> > zoecarver wrote:
> > > cjdb wrote:
> > > > zoecarver wrote:
> > > > > ldionne wrote:
> > > > > > cjdb wrote:
> > > > > > > zoecarver wrote:
> > > > > > > > No need to call assert here. 
> > > > > > > Don't we want to check the run-time value as well?
> > > > > > Yes, I think we do. I'm leaving the `assert` in place.
> > > > > Why do we want to check the runtime value? The end of `check_iter_move` is `return true;` we're not "checking" anything. 
> > > > Sure, but the contents of `check_iter_move` need to be evaluated at both compile-time and run-time.
> > > I agree. But why can't you just call `check_iter_move` normally? Why do you need the assert? 
> > +1 @zoecarver 
> Oh, you're talking about literally removing `assert`, not what's inside it. I'm okay with that!
Heh, sorry I wan't clear. Yes, that's what I mean :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99873/new/

https://reviews.llvm.org/D99873