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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/__ranges/drop_view.h:37
+
+    static constexpr bool _UseCache = forward_range<_View> && !random_access_range<_View>;
+    using _Cache = optional<iterator_t<_View>>;
----------------
tcanens wrote:
> The cache needs to be used for random access ranges too unless the range is also sized.
Why does the range being sized matter? Doesn't a random access range provide amortized constant-time complexity for element lookup (which is the requirement for `drop_view`)?


================
Comment at: libcxx/include/__ranges/drop_view.h:141-144
+  template<class _Range>
+  drop_view(_Range&&, range_difference_t<_Range>)
+  // TODO: this is just recreating all_t.
+    -> drop_view<decltype(views::all(std::declval<_Range>()))>;
----------------
cjdb wrote:
> Does this need to be fixed?
At some point, yes. But we don't have `all_t` yet and this is not functionally different than what it will be. 


================
Comment at: libcxx/include/__ranges/drop_view.h:41
+  template<view _View, bool = forward_range<_View>>
+  struct __cache_base {
+    iterator_t<_View> __cached_begin{};
----------------
Quuxplusone wrote:
> cjdb wrote:
> > `__cache_base` is a bit too general a name.
> > I'm going to leave it at the top
> 
> Please do as @ldionne says, and follow libc++ style. Private/data members go at the bottom (and data members get a suffix-underscore; thanks for addressing that part already).
> Also, please consider putting `__cached_begin_` in front of `__count_`, for layout reasons. It's more likely that `_View` begins with an `_Empty` member than that `_View::difference_type` does.
I reordered these based on Tim's suggestion:
> I'd put __count before __base - see https://lists.isocpp.org/lib/2020/09/17540.php.

But you're right, I think cached_begin should still come first (then count, then base). 


================
Comment at: libcxx/include/__ranges/drop_view.h:41
+  template<view _View, bool = forward_range<_View>>
+  struct __cache_base {
+    iterator_t<_View> __cached_begin{};
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > cjdb wrote:
> > > `__cache_base` is a bit too general a name.
> > > I'm going to leave it at the top
> > 
> > Please do as @ldionne says, and follow libc++ style. Private/data members go at the bottom (and data members get a suffix-underscore; thanks for addressing that part already).
> > Also, please consider putting `__cached_begin_` in front of `__count_`, for layout reasons. It's more likely that `_View` begins with an `_Empty` member than that `_View::difference_type` does.
> I reordered these based on Tim's suggestion:
> > I'd put __count before __base - see https://lists.isocpp.org/lib/2020/09/17540.php.
> 
> But you're right, I think cached_begin should still come first (then count, then base). 
> Private/data members go at the bottom (and data members get a suffix-underscore; thanks for addressing that part already).

Where does it say that private data members go at the bottom? If it's not documented anywhere, I'll go off of what style we currently use. Doing a quick audit of the first three types that come to mind, I see that `basic_string`, `shared_ptr` (and all of its helper classes), and `map` all put their data members at the top. After I continued to look, the only class I could find that didn't do this is `vector`, but even its base/helper classes put their private members at the top. 

I find these comments about formatting (here and elsewhere) counter productive. I try not to comment on formatting unless it's really egregious. Until we create some formatting rules (which I think would be a great idea, because then contributors would know how to format their code, and you wouldn't have to waste time in reviews asking for changes), let's not comment about formatting unless it makes the code harder to understand. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102037