[libcxx-commits] [PATCH] D102434: [libcxx][ranges] adds concept `sized_range` and cleans up `ranges::size`

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked an inline comment as done.
cjdb added inline comments.


================
Comment at: libcxx/include/__ranges/concepts.h:10
 #ifndef _LIBCPP_RANGES_CONCEPTS_H
 #define _LIBCPP_RANGES_CONCEPTS_H
 
----------------
Mordante wrote:
> Not part of your changes, but can you use a triple underscore? `_LIBCPP_RANGES_CONCEPTS_H` -> `_LIBCPP___RANGES_CONCEPTS_H`
Will do.


================
Comment at: libcxx/include/__ranges/concepts.h:14
+#include <__iterator/concepts.h>
+#include <__ranges/access.h>
+#include <__ranges/size.h>
----------------
cjdb wrote:
> zoecarver wrote:
> > How was this working before? 
> 🤷
Oh, because of transitive includes.


================
Comment at: libcxx/test/std/ranges/range.req/range.sized/sized_range.compile.pass.cpp:49
+static_assert(stdr::sized_range<const_range_has_size>);
+static_assert(stdr::range<const_range_has_size const>);
+static_assert(!stdr::sized_range<const_range_has_size const>);
----------------
Mordante wrote:
> Why is this the only one to check `stdr::range`?
Confirming it's a range, but not a sized range.


================
Comment at: libcxx/test/std/strings/string.view/range_concept_conformance.compile.pass.cpp:31
 static_assert(stdr::random_access_range<std::string_view const>);
 static_assert(!stdr::view<std::string_view const>);
+static_assert(stdr::sized_range<std::string_view const>);
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > cjdb wrote:
> > > Quuxplusone wrote:
> > > > Unrelated to this PR, but it strikes me as odd that `string_view` is //not// a `std::ranges::view`.
> > > > Should you perhaps be asserting `stdr::view<std::string_view&>` instead?
> > > That would be because we haven't enabled `string_view` as a view yet.
> > Hmm. I think we should at least have a comment saying that this test is wrong. I'd even say we should comment out the static_assert too (and make it positive). 
> @zoecarver: If we comment out the assert, we'll have to set some kind of watchdog to remind us to comment it back in, in the PR that enables `string_view`-as-a-view. Which will definitely fail to remind us, and then tech debt.
> By keeping this "wrong" assert enabled, it serves as a regression test: it serves as its own infallible watchdog. When `string_view` becomes a view, buildkite will point //directly at this test file//, telling us that we need to flip the sense of the assert as part of the same PR. That's excellent. (And also excellent to see in the git history later, when we're wondering which commit caused `string_view` to become a view.)
> I wouldn't mind seeing a `// TODO: string_view is nonconforming` comment at the end of this line, though.
> 
> A possible alternative approach (but I assume we won't block D102434 on this): //first// land a PR that makes `string_view`-a-view; //then// land this PR with the assert already passing.
> @zoecarver: If we comment out the assert, we'll have to set some kind of watchdog to remind us to comment it back in, in the PR that enables `string_view`-as-a-view. Which will definitely fail to remind us, and then tech debt.
> By keeping this "wrong" assert enabled, it serves as a regression test: it serves as its own infallible watchdog. When `string_view` becomes a view, buildkite will point //directly at this test file//, telling us that we need to flip the sense of the assert as part of the same PR. That's excellent. (And also excellent to see in the git history later, when we're wondering which commit caused `string_view` to become a view.)

+1

> I wouldn't mind seeing a `// TODO: string_view is nonconforming` comment at the end of this line, though.
> 
> A possible alternative approach (but I assume we won't block D102434 on this): //first// land a PR that makes `string_view`-a-view; //then// land this PR with the assert already passing.

Sure, I can add that `TODO` before merging (it won't appear on Phab though).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102434