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

Wed Sep 29 10:06:39 PDT 2021

ldionne 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:
> Quuxplusone wrote:
> > 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.)
> Hmm I see my post was a bit terse. But I meant:
> * Keep the file at its current location.
> * Remove the `REQUIRES: libc++`
Okay, reading this discussion made me change my mind, I guess. There are some misconceptions. `verify.cpp` tests are *not* libc++ specific in spirit. `verify.cpp` tests are simply a type of test supported by the testing format, and I don't think it makes sense to tie them to a specific implementation of the C++ stdlib.

Also, they are not Clang specific *in spirit*. It is true that we don't support them with any other compiler right now because we need a compiler that supports `clang-verify`, but in theory it would be conceivable to support `verify.cpp` tests for other compilers. There would also be issues with how to translate diagnostics and all, but my point is that in spirit there's nothing Clang or libc++ specific with those tests.

So what I'm going to do is move those libc++ specific tests to `libcxx/test/libcxx`, and also do a pass at our existing tests to move the libc++ specific ones to the right location.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp:60
+  {
+    using SomeView = NonCommonView;
+
----------------
Quuxplusone wrote:
> Why introduce this typedef? Couldn't you just use `NonCommonView` throughout, below?
Cause I wanted to capture the fact that it's just "some view" -- I didn't pick `NonCommonView` specifically, it could have been a `CommonView` too.

================
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);
----------------
Quuxplusone wrote:
> 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)>>);
> ```
In this case you're right because `Result = .....` is long so I put it on its own line, but in general it does reduce duplication in the tests and it can be easier to read. I'm not strongly attached to it, but I kind of liked it when (I think) @cjdb mentioned it to me, so I started adopting it. I'll keep doing that unless we come to an agreement that it's inferior.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp:93
+  {
+    static_assert(std::same_as<decltype(std::views::common), decltype(std::ranges::views::common)>);
+  }
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > Not only that, but `std::addressof(std::views::common) == std::addressof(std::ranges::views::common)`.
> > > I don't know if we really care about testing this; but if we do, then perhaps we should test //all// the members of namespace `views`, in one single test file, together.
> > We're not supposed to be able to take the address of those so I think it's reasonable not to test for that.
> I agree; but I meant that right now you're just testing that `views::x` and `ranges::views::x` have the same //type//; you're not actually testing that they are //two different names for the same object// (which is what we want).
> You can definitely pass `std::views::common` by reference to a function — that is a deliberate supported use-case — so you can get its address by using `addressof` on that reference. I don't know where we draw the line between "manipulating an object by reference such that you obtain a pointer to the object" (which is fine) and "taking the object's address" (which is allegedly forbidden). ;)
> Anyway, minor point. I still think that if we're spending brain cells on this, we should do it for //all// the views, in one place, not scattered. And if we're not spending brain cells on it, then ship it. :)
I'll colocate those tests in an upcoming patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110433