[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/__memory/ranges_uninitialized_algorithms.h:37
+
+struct __uninitialized_default_construct_fn final : private __function_like {
+
----------------
Quuxplusone wrote:
> This should be as in the blog post too:
> ```
> namespace __uninitialized_default_construct {
> struct __fn {
> };
> } // namespace __uninitialized_default_construct
> ```
> AIUI, this keeps the CPO's own type from ADL'ing into the `std::ranges` namespace; e.g. `foobar(std::ranges::uninitialized_default_construct)` should not consider `std::ranges::foobar` a candidate //even if std::ranges::foobar is not a CPO itself.//
> Also, of course, consistency (Chesterton's Fence, the economist's hundred-dollar bill): if it were safe to omit the namespace, we'd certainly want to do it everywhere, not just here.
Done (I added an `_ns` suffix to the namespace name to make sure it's different from the name of the helper functions). Do we actually need a separate namespace for each CPO, though?

By the way, can you please elaborate on how this could be a problem, given that the type of the CPO is an underscore-prefixed internal type?


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


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/uninitialized_default_construct.pass.cpp:90
 
-void test_value_initialized()
-{
----------------
Quuxplusone wrote:
> Btw, please commit this removal in a separate commit, and/or make sure there's something in your commit message about how we believe this was UB and that's why it's gone now.
Updated the commit message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115315