[libcxx-commits] [PATCH] D110433: [libc++] Add the std::views::common range adaptor

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__ranges/common_view.h:111
+    constexpr auto operator()(_Range&& __range) const
+      noexcept(noexcept(views::all(_VSTD::forward<_Range>(__range))))
+      -> decltype(      views::all(_VSTD::forward<_Range>(__range)))
----------------
Shouldn't there be tests to verify the `noexcept` status of the adaptor?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.nodiscard.verify.cpp:12
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
+// REQUIRES: libc++
+
----------------
jloser wrote:
> ldionne wrote:
> > jloser wrote:
> > > Question: since this is a `libc++` extension, should we move this file into `libcxx/test/libcxx` for example?
> > Yes, the convention would be to put those in `libcxx/test/libcxx`. However, I've been sort of on the fence about this recently, it feels easier and clearer to put those alongside the normal tests and just use `REQUIRES: libc++` to express that it's a libc++ specific test. The benefits I see are that you don't have to search for tests in two directories, and also there's no risk of the folder hierarchies getting out of sync if they get moved around.
> > 
> > I'll move it to `libcxx/test/libcxx` if you request it, otherwise I'll leave as-is.
> FWIW, I'm OK leaving it where it is now alongside the other tests. I prefer that, but will point out we have some inconsistency it seems. We can clean it up at some point in the future if desired (I'd be in favor of it).
But being a `verify` test already implies a `libc++` test. I've seen those more often in the `std` directory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110433