[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/test/std/ranges/range.adaptors/range.common.view/adaptor.nodiscard.verify.cpp:12
+// UNSUPPORTED: libcpp-has-no-incomplete-ranges
+// REQUIRES: libc++
+
----------------
ldionne wrote:
> 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.
Thanks for the clarification and moving the appropriate tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110433