[libcxx-commits] [PATCH] D142335: [libc++][ranges] Implement `ranges::to`.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked 13 inline comments as done.
var-const added inline comments.


================
Comment at: libcxx/include/__ranges/to.h:47
+
+// Note: in the Standard, `reservable-container` is a `constexpr bool` variable. However, GCC has a compiler bug that
+// makes short-circuiting not work properly in that case, so use a concept as a workaround.
----------------
philnik wrote:
> Do you know whether GCC 13 fixes that bug? If yes, maybe make this a TODO, since we will switch very soon?
Hmm, this was a while ago, but looking up my notes, it looks like I was conflating two issues here. The compiler with a bug in this case was Clang 15: https://godbolt.org/z/aGrE1oP1v. I'll switch back to using a `constexpr bool` variable for consistency with the Standard and see if anything breaks on the CI. Thanks for calling the attention to this!


================
Comment at: libcxx/include/deque:1823
+deque<_Tp, _Allocator>::__insert_bidirectional(const_iterator __p, _BiIter __f, _Sentinel __sent, size_type __n) {
+    auto __l = std::ranges::next(__f, __sent);
     size_type __pos = __p - begin();
----------------
ldionne wrote:
> This does have the same problem that we're re-walking the whole input sequence again in some cases, let's see if there's a solution for that.
Hmm, how about using two overloads, one where the type of `__l` is an exact match for `_BiIter` and one when it's a different sentinel type? That way, we can only call `next` when the types differ (and thus `next` cannot be reduced to simply return `__sent`).


================
Comment at: libcxx/include/vector:1478-1479
         {
-            __growing = true;
-            __mid =  __first;
-            std::advance(__mid, size());
+            std::advance(__first, size());
+            __construct_at_end(__first, __last, __new_size - size());
+
----------------
ldionne wrote:
> var-const wrote:
> > ldionne wrote:
> > > This doesn't seem to work, I think we're consuming the first `size()` elements from `__first` and then copying the rest into the vector only. I think this also means there's a gap in the testing.
> > Thanks! I think this is the right form:
> > ```
> > if (__new_size <= capacity()) {
> >   if (__new_size > size()) {
> >     _ForwardIterator __mid = __first;
> >     std::advance(__mid, size());
> > 
> >     std::copy(__first, __mid, this->__begin_);
> >     __construct_at_end(__mid, __last, __new_size - size());
> > 
> >   } else {
> >     pointer __m = std::copy(__first, __last, this->__begin_);
> >     this->__destruct_at_end(__m);
> >   }
> > }
> > ```
> > I think it's actually more readable without the `__growing` variable.
> ```
> if (__new_size <= capacity()) {
>   if (__new_size > size()) {
>     _ForwardIterator __mid = std::next(__first, size());
> 
>     std::copy(__first, __mid, this->__begin_);
>     __construct_at_end(__mid, __last, __new_size - size());
> 
>   } else {
>     pointer __m = std::copy(__first, __last, this->__begin_);
>     this->__destruct_at_end(__m);
>   }
> }
> ```
> 
> Yeah, I think that works!
Note: still need to fix the testing part.


================
Comment at: libcxx/include/vector:1921
 {
-    _LIBCPP_DEBUG_ASSERT(__get_const_db()->__find_c_from_i(std::addressof(__position)) == this,
-                         "vector::insert(iterator, range) called with an iterator not referring to this vector");
+  _LIBCPP_DEBUG_ASSERT(__get_const_db()->__find_c_from_i(std::addressof(__position)) == this,
+                        "vector::insert(iterator, range) called with an iterator not referring to this vector");
----------------
ldionne wrote:
> This should be in `__insert_with_sentinel`, I think.
I think I originally left it there due to the way the error message is written (referring to the exact name of the invoking function). But I don't think we lose much by making it slightly more generic, not to mention that the debug mode is effectively non-operational anyway.


================
Comment at: libcxx/test/std/containers/insert_range_helpers.h:31
+
+// A simple literal-type container. It can be used as a `constexpr` global variable (which isn't supported by
+// `std::vector`).
----------------
ldionne wrote:
> I think you don't need this class if you don't store your test cases in global variables. Then you could simply use `std::string` and it would work in `constexpr` too. IMO that would be better since this class is very specific.
I tried changing this but not sure it's worth it. `constexpr` variables have a very nice property that if one of them is accidentally unused (i.e. a test case is omitted), the compiler produces an error. The class is pretty small and I don't think it adds that much maintenance burden. I remember implementing something similar before to work around some other limitations of `vector` and `string` which makes me suspect that having full control over the class's implementation has some value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142335