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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/__ranges/drop_view.h:49
+public:
+    constexpr drop_view() noexcept(noexcept(_View()))
+      requires default_initializable<_View>
----------------
ldionne wrote:
> `noexcept(is_nothrow_default_constructible_v<_View>)`?
> 
> Also, this should be tested.
Why are we not just `= default`ing this constructor?


================
Comment at: libcxx/include/__ranges/drop_view.h:133
+    constexpr auto size()
+      requires sized_range<_View>
+    { return __size(*this); }
----------------
ldionne wrote:
> 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?
> is there a reason why you went for no indentation in the first place?

It's the style I adopted back in 2016 when working on cmcstl2; libc++ didn't have any established precedent for requires-clauses, so I paved a way forward. I also find it kind of arbitrary to indent it? We don't indent in the middle of a code block unless we open some sort of grouping tokens (usually parens or braces).
It also swallows up valuable horizontal width, which can impact readability.

> Is there an established way of formatting those in the community?

* cmcstl2 and cjdb-ranges line it up
* Microsoft/STL and libstdc++ indent

All four use same-line requires-clauses for short requirements, which I think we should adopt in libc++.

> I had not realized that the WP was indenting requires-clauses, but I think it might make sense to try to follow that. 

Sure, if you want to rescind your offer from D99691, I'll play ball. I would like to know why you think it makes sense to follow the standard wording, but we can follow up with that offline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102037