[libcxx-commits] [PATCH] D115315: [libc++][ranges] Implement ranges::uninitialized_default_construct{, _n}.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__iterator/advance.h:72
 // [range.iter.op.advance]
+// TODO(varconst): rename `__advance_fn` to `__fn`.
 struct __advance_fn final : private __function_like {
----------------
Quuxplusone wrote:
> And put it in the `__advance` namespace, and put `advance` into `inline namespace __cpo`; and remove `__function_like`. :)  If you're volunteering for this, I will be thrilled to review it. :)
Thanks, I plan to do this cleanup before the end of the week.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/special_function.compile.pass.cpp:91
 
+// TODO(varconst): simply check that `next` is a variable and not a function.
 // When found by unqualified ([basic.lookup.unqual]) name lookup for the postfix-expression in a
----------------
Quuxplusone wrote:
> var-const wrote:
> > Quuxplusone wrote:
> > > Yay. :) IMHO a more helpful pattern for this comment would just be
> > > `// TODO: make consistent with std/iterato...compile.pass.cpp`
> > > and name one of the files that you consider to be correct. Then, even someone-not-you should be able to deduce exactly what this comment is asking them to do.
> > I don't feel too strongly about this, but in general I'm a little wary about comments that (effectively) contain links to other code -- the linked code can change and the comment becomes stale (e.g. if in the future these checks were to be moved to a single file).
> > 
> > Perhaps it would be even better to combine both approaches (explain the intent _and_ link an example), but given that I plan to fix this in the next few days, it seems like overkill to me.
> Ack; OK if you leave this TODO comment as-is.
> For the record, though, re "the linked code can change and the comment becomes stale": the benefit of wording the comment as "make consistent with [linked code]" is that even if the linked code //does// change, the comment remains correct!  (Unless the linked code changes to match //this// code. ;))  A comment like "Here we should take approach X" might rot (if tomorrow we standardize on approach Y), but "Here we should be consistent and take the same approach as our neighbor" is evergreen.
Sorry, I should have elaborated a bit more -- the kind of change I was thinking about is if the linked code was moved around (e.g. imagine in the future the code that checks CPO object properties is consolidated in a single file -- then the reader won't find anything in the test file that is "linked" here and would have to go dig through history to figure out what the comment was referring to).


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct.pass.cpp:210-211
+    std::ranges::uninitialized_default_construct(buf.cbegin(), buf.cend());
+    assert(Counted::current_objects == buf.capacity());
+    assert(Counted::total_objects == buf.capacity());
+    std::destroy(buf.begin(), buf.end());
----------------
Quuxplusone wrote:
> Nit: Using the name `capacity` here instead of `size` is weird (compare to `vector.size/capacity`); but also can we just say `5` to sidestep the whole issue? (Applies throughout, I guess.)
Created a constant instead.

Regarding the naming issue: my initial thought was that what `capacity` returns essentially is just raw capacity, not size. The buffer allocates enough space for `N` elements, but it doesn't keep track of how many elements are actually created, which is what I would expect `size` to return.

OTOH, `size` in the vector sense arguably is inapplicable for `Buffer` because `Buffer` allows random access to all the `N` elements -- e.g. for a 5-element buffer, it's valid to have elements `[1]` and `[3]` initialized and everything else uninitialized, and there isn't really a way to capture that information as `size`.

Now that I think of it, it seems like `std::array` is a better model for `Buffer` than vector, and array uses `size`, not `capacity`.


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct.pass.cpp:218-221
+    Buffer<Counted, 5> buf;
+    auto range = std::ranges::subrange(buf.cbegin(), buf.cend());
+
+    std::ranges::uninitialized_default_construct(range);
----------------
Quuxplusone wrote:
> FWIW, if you want, and if you rename `Buffer.cbegin/cend` to `begin const/end const` so that `Buffer` becomes more range-ish, then this can just be
> ```
> std::ranges::uninitialized_default_construct(std::as_const(buf));
> ```
Thanks! I'll experiment with this in the follow-up I'm working on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115315