[libcxx-commits] [PATCH] D118736: [libc++] Guard std::ranges under _LIBCPP_HAS_NO_INCOMPLETE_RANGES.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/libcxx/ranges/has-no-incomplete-ranges.compile.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
ldionne wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > I don't think this test adds that much value -- instead, we should just add a CI configuration that tests without incomplete features, and also make sure not to needlessly mark tests as `UNSUPPORTED` (like we did for the span ones).
> > We should definitely add a CI configuration that includes `libcpp-has-no-incomplete-ranges` and/or `libcpp-has-no-incomplete-format`. However, this test is the only test that actually tests the //point// of that config flag: that, when it is enabled, you //don't get// these features. Otherwise, we could just remove the macro from the entire codebase (or misspell it) and CI would never notice that the features had become enabled.
> > (Like how right now `std::ranges::advance` is enabled: do we want it to be? Maybe we do, in which case this test is not harmless but actually testing something we //don't want//. But we should consciously decide that.)
> Okay, I agree. Can we instead use a `.verify.cpp` test that does `namespace rng = std::ranges` and checks for an error? As it stands, the test is technically IFNDR.
> 
> Also, regarding testing a configuration without incomplete features, see https://reviews.llvm.org/D118740.
> Can we instead use a .verify.cpp test that does namespace rng = std::ranges and checks for an error?

Sure, I suppose. The reason I did it this way specifically is that I needed to use the compiler diagnostics on failure to detect exactly which header(s) were still defining `namespace ranges`. (The accompanying note points to the offending `namespace ranges` line.) Just seeing "Expected an error and didn't get one" gave no clue as to where the problem was coming from, so I wasn't able to iterate on that. But if we keep this clean going forward, then the lack of helpful hints shouldn't be a huge deal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118736