[libcxx-commits] [PATCH] D123600: [libc++][ranges] Implement `views::take`.

Wed May 4 02:02:29 PDT 2022

var-const added inline comments.

================
Comment at: libcxx/include/__fwd/string_view.h:10
+
+#ifndef _LIBCPP_FWD_STRING_VIEW_H
+#define _LIBCPP_FWD_STRING_VIEW_H
----------------
philnik wrote:
> Just so you don't miss them when moving the headers.
Thanks a lot!

================
Comment at: libcxx/include/__fwd/string_view.h:1
+// -*- C++ -*-
+//===---------------------------------------------------------------------===//
----------------
philnik wrote:
> var-const wrote:
> > huixie90 wrote:
> > > philnik wrote:
> > > > huixie90 wrote:
> > > > > ldionne wrote:
> > > > > > philnik wrote:
> > > > > > > `__string_view/fwd.h` would make more sense I think. Same for `span`.
> > > > > > FWIW I don't really have a strong opinion. The only downside to this is that we might end up with a lot of one-header subdirectories if we start adding forward declarations to more classes, and putting everything under `__fwd/` would avoid that.
> > > > > I am in favour of fwd folder. I don’t like to have too many files with the exact same name fwd.hpp 
> > > > I don't like having a `__fwd` subdirectory because that would mean we have a single header spread across multiple subdirecories. That makes it a lot harder to see what is part of a header.
> > > I think having a single fwd folder whose subdirectories hierarchies follow the non-fwd hierarchies is pretty neat. You can easily find the forward declarations by adding a fwd at the beginning. One example code base is Boost.Hana 
> > I'm slightly in favor of having all the forward declarations under a single folder because it would make it easier to see what can and cannot be forward-declared. While having contents of a single header spread across several directories in general would be bad, I think splitting out forward declarations following a consistent pattern shouldn't cause any issues. @philnik How strongly do you feel about this?
> The reason you want to forward declare the things here is to avoid including all of the header and having longer compile times, right? If there is another technical reason for that, please tell me! So there shouldn't be any reason for new headers to have them in 2-3 years (hopefully).  I don't think there will be a lot of new forward declared headers because of this, so I think having forward declarations in another directory is quite surprising.
> So yeah, I do feel quite strongly about this.
@philnik Like Louis said, having some forward declarations is probably unavoidable. But even if we were to remove them in the future, it seems like it would still be (very slightly) easier if they're all within a single folder. In any case, changing this structure in either direction is very straightforward, so we can always change our solution later on with minimal hassle. I'm inclined to keep as is -- if you still feel strongly, I can do a follow-up (feel free to ping me on Discord if that's the case).

================
Comment at: libcxx/include/__ranges/take_view.h:235
+  // [range.take.overview] 2.1
+  template <class _Range, convertible_to<range_difference_t<_Range>> _Np>
+    requires __is_empty_view<remove_cvref_t<_Range>>
----------------
huixie90 wrote:
> I think it should be the different type of `_Range&&` instead of `_Range`
In this case, I don't think it matters. `range_difference_t<T>` depends on `iterator_t<T>`, which in turn is defined as `decltype(ranges::begin(std::declval<T&>()))`. I think the type of `T&` would be the same regardless of whether `T` is `_Range` or `_Range&&`. It's a good question in general, though -- please let me know if you see a similar pattern where this could make a difference.

================
Comment at: libcxx/include/__ranges/take_view.h:255-256
+    noexcept(noexcept(__passthrough_type_t<_RawRng>(
+                              ranges::begin(std::forward<_Rng>(__rng)),
+                              ranges::begin(std::forward<_Rng>(__rng)) +
+                                  std::min<_Dist>(ranges::distance(std::forward<_Rng>(__rng)), std::forward<_Np>(__n))
----------------
ldionne wrote:
> It is not clear to me that we want to use `std::forward` here. Indeed, http://eel.is/c++draft/range.take#view does say `U(ranges::begin(E), ranges::begin(E) + std::min<D>(ranges::distance(E), F))` where `E` is the expression in `views::take(E, F)`. However, we generally want to avoid forwarding in more than one expression at a time because that sets us up for double-moves. In this case it should not matter since `begin` has to take either `T&` or `T const&`, but I think I'd drop the `forward` here. IIUC, the same is true for `ranges::distance`.
Done, thanks for calling this out!

================
Comment at: libcxx/include/__ranges/take_view.h:259
+                              )))
+    -> decltype(      __passthrough_type_t<_RawRng>(
+                              ranges::begin(std::forward<_Rng>(__rng)),
----------------
huixie90 wrote:
>  question : the spec doesn’t say “if the expression is ill formed, goto otherwise clause”. do we need sfinae here? 
The way I interpret it is that it says "If the type satisfies such and such condition, do this, otherwise go to the next clause". If the resulting expression is ill-formed, then the type does not satisfy the condition, so it should check the next clause. Sorry if I misunderstood the question -- in that case, please elaborate a little.

================
Comment at: libcxx/include/__ranges/take_view.h:306
+  template <class _Np>
+    requires constructible_from<decay_t<_Np>, _Np>
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
----------------
huixie90 wrote:
> Should the second _Np be _Np&& ?
I don't think so. Do you have a case in mind where it would make a difference?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123600