[libcxx-commits] [PATCH] D149830: [libc++][ranges] Implement the changes to node-based containers from P1206 (`ranges::to`):

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/list:786-788
+      for (auto&& __e : __range) {
+        __emplace_back(std::forward<decltype(__e)>(__e));
+      }
----------------
ldionne wrote:
> var-const wrote:
> > ldionne wrote:
> > > Maybe we can use `prepend_range(...)` here instead? It would make it the same as what we do for `forward_list`.
> > I copied this implementation from the existing constructor taking 2 iterators. The amount of work between the two possible implementations is comparable, but the implementation relying on insertion contains exception cleanup code that isn't in `emplace_back`.  However, despite that, a quick benchmark shows that the difference between the two implementations seems to be within fluctuation (attached below). 
> > 
> > Given these results, `prepend_range` seems preferable as it's slightly simpler and also consistent with `forward_list` like you mentioned. However, let's discuss whether we want the extra exception cleanup code.
> > 
> > `prepend_range` version:
> > ```
> >   -----------------------------------------------------------------------------
> >   Benchmark                                   Time             CPU   Iterations
> >   -----------------------------------------------------------------------------
> >   BM_ConstructRange/list_char/1024        20033 ns        20008 ns        36195
> >   BM_ConstructRange/list_size_t/1024      20726 ns        20704 ns        35864
> >   BM_ConstructRange/list_string/1024     105972 ns       105897 ns         6653
> >   -----------------------------------------------------------------------------
> >   Benchmark                                   Time             CPU   Iterations
> >   -----------------------------------------------------------------------------
> >   BM_ConstructRange/list_char/1024        20545 ns        20520 ns        30527
> >   BM_ConstructRange/list_size_t/1024      20966 ns        20949 ns        35016
> >   BM_ConstructRange/list_string/1024     106834 ns       106774 ns         6637
> >   -----------------------------------------------------------------------------
> >   Benchmark                                   Time             CPU   Iterations
> >   -----------------------------------------------------------------------------
> >   BM_ConstructRange/list_char/1024        20083 ns        20069 ns        36502
> >   BM_ConstructRange/list_size_t/1024      20106 ns        20090 ns        35770
> >   BM_ConstructRange/list_string/1024     105067 ns       104821 ns         6891
> >   -----------------------------------------------------------------------------
> >   Benchmark                                   Time             CPU   Iterations
> >   -----------------------------------------------------------------------------
> >   BM_ConstructRange/list_char/1024        19772 ns        19754 ns        36000
> >   BM_ConstructRange/list_size_t/1024      21022 ns        20775 ns        34938
> >   BM_ConstructRange/list_string/1024     104036 ns       103966 ns         6803
> > ```
> > 
> > `emplace_back` version:
> > ```
> >   -----------------------------------------------------------------------------
> >   Benchmark                                   Time             CPU   Iterations
> >   -----------------------------------------------------------------------------
> >   BM_ConstructRange/list_char/1024        20099 ns        20076 ns        35770
> >   BM_ConstructRange/list_size_t/1024      20770 ns        20742 ns        35921
> >   BM_ConstructRange/list_string/1024     108025 ns       107948 ns         6793
> >   -----------------------------------------------------------------------------
> >   Benchmark                                   Time             CPU   Iterations
> >   -----------------------------------------------------------------------------
> >   BM_ConstructRange/list_char/1024        19628 ns        19598 ns        36649
> >   BM_ConstructRange/list_size_t/1024      19334 ns        19313 ns        36364
> >   BM_ConstructRange/list_string/1024     108159 ns       108050 ns         6654
> >   -----------------------------------------------------------------------------
> >   Benchmark                                   Time             CPU   Iterations
> >   -----------------------------------------------------------------------------
> >   BM_ConstructRange/list_char/1024        19960 ns        19945 ns        36955
> >   BM_ConstructRange/list_size_t/1024      19963 ns        19943 ns        35697
> >   BM_ConstructRange/list_string/1024     108664 ns       108599 ns         6715
> >   -----------------------------------------------------------------------------
> >   Benchmark                                   Time             CPU   Iterations
> >   -----------------------------------------------------------------------------
> >   BM_ConstructRange/list_char/1024        19666 ns        19651 ns        36989
> >   BM_ConstructRange/list_size_t/1024      19662 ns        19642 ns        35865
> >   BM_ConstructRange/list_string/1024     106818 ns       106769 ns         6660
> > ```
> If `prepend_range` and the other operations have the strong exception safety guarantee, we should make sure to test that.
> 
> Regarding whether to use `prepend_range` or not in this constructor, I think we should, but only for consistency, since the strong exception safety guarantee isn't relevant.
Added a test similar to what I recently added for `forward_list` (and also added the new range methods to the `forward_list` test).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149830