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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

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

Looks good to me at this point!



================
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 {
----------------
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. :)


================
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
----------------
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.


================
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());
----------------
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.)


================
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);
----------------
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));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115315