[libcxx-commits] [PATCH] D104262: [libcxx][ranges] Add contiguous_range.
Christopher Di Bella via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Jun 23 17:41:51 PDT 2021
cjdb added inline comments.
================
Comment at: libcxx/test/std/ranges/range.req/range.refinements/contiguous_range.compile.pass.cpp:42-46
+struct BadDataMember {
+ int *begin();
+ int *end();
+ char *data();
+};
----------------
zoecarver wrote:
> cjdb wrote:
> > Quuxplusone wrote:
> > > I was initially confused by the name, as this class has //no// data members... but then I saw: the trick is that `X::data()` member function has a return type that is different from `std::add_pointer_t<ranges::range_reference_t<X>>`.
> > > I think it'd be worth replacing this test (or at augmenting it) with both of these:
> > > ```
> > > struct WrongConstness {
> > > const int *begin() const;
> > > const int *end() const;
> > > int *data() const;
> > > };
> > > static_assert(std::ranges::random_access_range<WrongConstness>);
> > > static_assert(!std::ranges::contiguous_range<WrongConstness>);
> > >
> > > struct NotPtr {
> > > contiguous_iterator<const int*> begin() const;
> > > contiguous_iterator<const int*> end() const;
> > > contiguous_iterator<const int*> data() const;
> > > };
> > > static_assert(std::ranges::random_access_range<NotPtr>);
> > > static_assert(!std::ranges::contiguous_range<NotPtr>);
> > > ```
> > > I was initially confused by the name, as this class has //no// data members... but then I saw: the trick is that `X::data()` member function has a return type that is different from `std::add_pointer_t<ranges::range_reference_t<X>>`.
> >
> > I propose renaming to `BadDataMemberFunction`.
> >
> > We should also have a check for when a range can satisfy `contiguous_range<R>`, but not `contiguous_range<R const>`.
> Thanks for the suggestions. Arthur, I added your first test only (with some modification). The `NotPtr` test doesn't work because contiguous_iterator has an element_type.
This still isn't complete, because now we're missing a test case for when both `contiguous_range<T>` and `contiguous_range<T const>` are true, and one for when only `contiguous_range<T const>` is true.
================
Comment at: libcxx/test/std/ranges/range.req/range.refinements/subsumption.compile.pass.cpp:28
template<std::ranges::range R>
-requires std::input_iterator<std::ranges::iterator_t<R> >
+requires std::input_iterator<std::ranges::iterator_t<R>>
[[nodiscard]] constexpr bool check_input_range_subsumption() {
----------------
zoecarver wrote:
> cjdb wrote:
> > Quuxplusone wrote:
> > > Whitespace: Please indent `requires` by one level (here and throughout). E.g.
> > > ```
> > > template<...>
> > > requires ...
> > > bool foo() {
> > > return ...
> > > }
> > > ```
> > > It'd be nice to remove the [[nodiscard]]s while you're at it.
> > @ldionne [[ https://reviews.llvm.org/D99691#inline-938990 | left it up to me ]] to decide how libc++ formats requires clauses, and [[ https://reviews.llvm.org/D99691#inline-939014 | I decided that they shouldn't be indented ]].
> I'm going to match the existing format of this file. I would support a nfc commit that updated all the requires in this file.
Let's not take any action till Louis and I resolve our discussion.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104262/new/
https://reviews.llvm.org/D104262
More information about the libcxx-commits
mailing list