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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.nodiscard.verify.cpp:12
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
+// REQUIRES: libc++
+
----------------
Mordante wrote:
> 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.
@ldionne:
> 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

@mordante:
> But being a `verify` test already implies a libc++ test.

I think Louis's point makes sense; I'm fine with leaving .verify.cpp tests in the test/std/ directory, as long as the test runner — and all human contributors! — will understand that .verify.cpp implies "run only with Clang, and only with libc++."
I have one question about the proposed-new-status-quo, and two very minor items to put in the "against" column, just to show I've thought of them and rejected them:
- Question: In the proposed-new-status-quo, since `.verify.cpp` implies "libc++ and Clang," isn't `REQUIRES: libc++` redundant here? I think you should remove it.
- Minor con: Contributors often add new tests by grabbing an existing test and modifying it. Putting .verify.cpp tests next to .pass.cpp tests might make it slightly more likely that a contributor will accidentally grab and modify the wrong kind of test. (But this point is pretty weak.)
- Minor con: Each .verify.cpp test is kind of like a little TODO, because it identifies test coverage that is present for Clang but missing for other compilers. Each test/libcxx/ test is //kind of// like a little TODO, too, because it identifies test coverage for features that are in libc++ but lack standards-conformance for one reason or another. So putting all these little TODO items in one place might make it easier to see how big is the "backlog" or "gap" between libc++ and full conformance. (But `find test/ -name *.verify.cpp` is just as easy to type as `find test/libcxx/`, so this point is even weaker.)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp:60
+  {
+    using SomeView = NonCommonView;
+
----------------
Why introduce this typedef? Couldn't you just use `NonCommonView` throughout, below?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp:75-76
+      auto const partial = std::views::transform(f) | std::views::common;
+      using Result = std::ranges::common_view<std::ranges::transform_view<SomeView, decltype(f)>>;
+      std::same_as<Result> auto result = partial(view);
+      assert(result.base().base().begin_ == buf);
----------------
Just kvetching for the record: I see no benefit to this newfangled approach over the old-school no-`Result`-typedef approach of
```
auto result = partial(view);
ASSERT_SAME_TYPE(decltype(result), std::ranges::common_view<std::ranges::transform_view<SomeView, decltype(f)>>);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110433