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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM w/ comments applied and CI passing.



================
Comment at: libcxx/include/list:786-788
+      for (auto&& __e : __range) {
+        __emplace_back(std::forward<decltype(__e)>(__e));
+      }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149830