[libcxx-commits] [PATCH] D102037: [libcxx][views] Add drop_view.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I would like to see tests for the constructors. They're going to be simple tests, but we should be testing everything systematically. Elsewhere we have `xxxx.ctor.pass.cpp` tests. Also, can you please explicitly mark where you're testing each overload of `begin()` and `end()`?

I've got a few comments, but they are pretty small. We should be able to land this today.



================
Comment at: libcxx/include/__ranges/drop_view.h:49
+public:
+    constexpr drop_view() noexcept(noexcept(_View()))
+      requires default_initializable<_View>
----------------
`noexcept(is_nothrow_default_constructible_v<_View>)`?

Also, this should be tested.


================
Comment at: libcxx/include/__ranges/drop_view.h:66
+      : __count_(__other.__count_)
+      , __cached_begin_() // Intentionally not propagating the cached begin iterator.
+      , __base_(__other.__base_)
----------------
I think `__cached_begin_(nullopt)` might be clearer - it's a nice reminder that we're not default-constructing the iterator itself, but only an `optional`. (here and elsewhere)


================
Comment at: libcxx/include/__ranges/drop_view.h:133
+    constexpr auto size()
+      requires sized_range<_View>
+    { return __size(*this); }
----------------
cjdb wrote:
> Quuxplusone wrote:
> > cjdb wrote:
> > > Please line up requires-clauses with the start of the function declaration. Small clause (such as this) can go on the same line.
> > > 
> > > I've usually turned a blind eye to this because comments about formatting are noise that lead to unproductive debates, and my intention has been to get clang-format to take care of this inconsistency in one fell swoop. Having said that, since I don't know when I'll be able to apply the tool project-wide and not get something weird, it's probably worth adopting the "official" libc++ style for future contributors' sakes.
> > FWIW, I agree about "unproductive debates" ;) but if there's an official libc++ style, it's always been "indent the subordinate clause by one level."
> > ```
> > template<class T>
> >   requires foo<T>
> > auto bar()
> >   -> decltype(baz)
> > {
> >   return quux;
> > }
> > ```
> > I don't think your insistence on following clang-format's (broken) formatting guidance here is helpful to the project. I predict that clang-format will eventually implement C++20 formatting, and then it'll be good that Zoe's been following the "indent requires-clauses" style.
> [[ https://reviews.llvm.org/D99691#inline-938990 | Louis gave me creative freedom ]] to set the style for requires-clauses. [[ https://reviews.llvm.org/D99691#inline-939014 |  I made the decision that they wouldn't be indented ]].
I had not realized that the WP was indenting requires-clauses, but I think it might make sense to try to follow that. @cjdb is there a reason why you went for no indentation in the first place?

What I care most about is that we're consistent and that our code is readable and idiomatic as much as possible. Is there an established way of formatting those in the community?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/begin.pass.cpp:13
+
+// class std::ranges::drop_view;
+
----------------
Those should describe what's being tested in the test. We've always done that in libc++, and frankly it's quite useful to know exactly what's being tested (especially since we love to have subtly different overloads of functions in the stdlib).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102037