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

Wed Dec 8 11:49:51 PST 2021

ldionne requested changes to this revision.
ldionne added a comment.

Overall LGTM with some comments. Thanks a lot, this is awesome and I love the direction we are taking for these algorithms.

================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:8
+//
+//===----------------------------------------------------------------------===//
+
----------------
Quuxplusone wrote:
> I'm still weakly inclined to split this up into `ranges_uninitialized_default_construct.h`, `ranges_uninitialized_value_construct.h`, `ranges_uninitialized_copy.h`, `ranges_uninitialized_move.h`, `ranges_uninitialized_fill.h`.
> 
> I imagine it's OK to have `ranges::foo` and `ranges::foo_n` in the same header, since the Standard actually does the same thing with its subsection naming (e.g. both `ranges::uninitialized_fill` and `ranges::uninitialized_fill_n` are located in https://eel.is/c++draft/uninitialized.fill ). We have two bad options here: smushing all ten niebloids into 1 header of 175 lines, or spreading them out into 5 headers of 80 lines each.
> 
> I'm weakly in favor of "5 headers" but I guess "1 header of 175 lines total" doesn't sound too bad when I put it that way.
I am mostly neutral on this, however I don't believe the different algorithms are going to have significantly different header dependencies. In that case, I think it might be simpler to just cram them into the same header -- after all they are closely related.

Like I said, no strong opinion here anyway.

================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:32
+
+struct __uninitialized_default_construct_fn final : private __function_like {
+
----------------
Quuxplusone wrote:
> I believe this `__function_like` business has caused weird Modules errors before (hence the workarounds you'll see in a few places in the modulemap). It's not needed for conformance, so I would strongly prefer you simply omit it — "Keep It Simple."
> Likewise I would personally omit the `final` cruft (it disables the EBO but otherwise has no effect except taking longer to read), but it's not going to cause weird Modules errors, so I don't care as much about whether or not we add `final`. :)
About `final` -- we have `[[no_unique_address]]` now so EBO shouldn't be an issue.

Regarding modules issues, basically it means you're going to need to export it from your private submodule, like so:

```
module ranges_uninitialized_algorithms {
    private header "__memory/ranges_uninitialized_algorithms.h"
    export __function_like
}
```

FWIW, I think there is some utility to removing things like addressability from our niebloids, the complexity around modules is just a bit annoying but it's still worth doing. Just my .02, this isn't anything close to "over my dead body".

================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:132
     for (; __idx != __last; ++__idx)
-        ::new ((void*)_VSTD::addressof(*__idx)) _Vt;
+        ::new ((void*)_VSTD::addressof(*__idx)) _ValueType;
 #ifndef _LIBCPP_NO_EXCEPTIONS
----------------
Quuxplusone wrote:
> var-const wrote:
> > Note: this cast is simpler than the [`voidify`](http://eel.is/c++draft/specialized.algorithms#general-4) function that the Standard mandates. Should I fix this, or is there a reason why a simple cast to `void*` is sufficient here?
> I can't think of any reason a simple cast to `void*` would be //insufficient//. I'd leave it simple until-and-unless someone comes up with a positive reason and gives us a regression test case. Otherwise, you'll complexify the code and someone six months from now will ask why it can't be simpler.
When @Quuxplusone originally removed `__voidify` in D93153, I was fine with it. I just did some additional research and I suspect it might have been wrong: https://githubmemory.com/repo/microsoft/STL/issues/1780

I think we should figure out what the situation is and either confirm that we're conforming, or fix all the places where we use `(void*)x` but the standard says `__voidify(x)`.

================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct.pass.cpp:10
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: gcc-10
----------------
Need to add `libcpp-has-no-incomplete-ranges`.

================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct.pass.cpp:11
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: gcc-10
+
----------------
We don't support it anymore, none of our tests mention this anymore unless I'm mistaken.

================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct.pass.cpp:60
+
+void test_empty() {
+  alignas(Counted) char buffer[sizeof(Counted) * 1] = {};
----------------
I've expressed this preference in other patches (not yours), but IMO it's better to use a comment to describe what we're testing, since it allows using plain english instead of trying to cram as much meaning as possible into a few identifiers (a function name). I would suggest:

```
void test() {
  // comment1
  {
    ...
  }

  // comment2
  {
    ...
  }

#ifndef TEST_HAS_NO_EXCEPTIONS
  {
    ...
  }
#endif
}
```

================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct.pass.cpp:62
+  alignas(Counted) char buffer[sizeof(Counted) * 1] = {};
+  auto* as_array = reinterpret_cast<Counted*>(buffer);
+
----------------
Non-binding, just a suggestion. I generally find it easier to glance at code when types are explicit since I don't have to parse and understand the expression used to initialize the variable in order to know its type. In this case it's arguably simple since it's a `reinterpret_cast`, but still.

Generally, I take `auto` as meaning: "you don't care about the specific type of the variable since it results from something more complex (like an expression template or a `views::foo(...)` call), so don't even try". When I see that, I basically assume that the precise type of the variable isn't important, only the properties of that type are. Here this isn't the case.

================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct.pass.cpp:103-116
+void test_already_initialized() {
+  int array[] = {42, 42, 42};
+
+  ranges::uninitialized_default_construct(std::begin(array), std::end(array));
+  assert(array[0] == 42);
+  assert(array[1] == 42);
+  assert(array[2] == 42);
----------------
Quuxplusone wrote:
> I'm pretty sure this is UB. `array[0]` is trivially destructible, so it's cool that you're skipping its destructor before creating a new `int` object at that location; but then you're //reading the value of// that new `int` object, and you haven't actually given it a value (default-constructing a primitive type doesn't set its value). So line 107 is UB.
> Just remove this whole function. Even if it weren't UB, I'm not clear on what it's trying to test.
> 
> (and yeah, I say the same thing about `test_value_initialized()` from `uninitialized_default_construct.pass.cpp`)
I actually buy @Quuxplusone 's reasoning as well.

Initially I thought we were verifying that we called the default constructor for trivially default constructible types, which does nothing (and hence doesn't overwrite the value). But I agree this looks like UB as well based on your reasoning.

IIUC, what we're *actually* trying to test here is that we are not calling any constructors. One way to do this would be to add a `.sh.cpp` test using `FileCheck` where we check the LLVM bitcode output, like they often do in Clang. I think it would be awesome to have that infrastructure available, however at the moment `FileCheck` isn't available while running the libc++ tests.

So IMO the pragmatic thing to do right now is to remove the test.

================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct.pass.cpp:147-152
+int main() {
+  test_empty();
+  test_several();
+  test_already_initialized();
+  test_exception_during_construction();
+}
----------------

================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct.pass.cpp:15-17
+// template <nothrow-forward-iterator ForwardIterator, nothrow-sentinel-for<ForwardIterator> Sentinel>
+//   requires default_initializable<iter_value_t<ForwardIterator>>
+// ForwardIterator ranges::uninitialized_default_construct(ForwardIterator first, Sentinel last);
----------------
Quuxplusone wrote:
> var-const wrote:
> > Is it necessary to list out all the constraints here, or would just `ForwardIterator ranges::uninitialized_default_construct(ForwardIterator first, Sentinel last)` (or even just `ranges::uninitialized_default_construct`) suffice?
> Personally, I'd err on the side of "just cut and paste what the Standard's synopsis says," which I believe is exactly what you've done here.
> 
> However, if you had just written `// ranges::uninitialized_default_construct`, I personally wouldn't have bothered to complain about it, either. :)
The only caveat to using `// ranges::uninitialized_default_construct` is that it's good to see the different overloads being tested. I really dislike seeing a test file that doesn't clearly say whether it tests all overloads of a function or not. In C++, different overloads often have different properties, and so they are really different functions entirely. They just happen to share the same "base name".

TLDR: IMO the current approach (copy-paste synopsis) is the best.

================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/special_function.compile.pass.cpp:24-55
+namespace found_by_adl {
+
+template <class I = int*>
+struct adl_forward_iterator : forward_iterator<I> {
+  // To satisfy `weakly_incrementable`, the increment operators must return the same type as the
+  // class.
+  adl_forward_iterator& operator++();
----------------
Quuxplusone wrote:
> This seems like overkill. (Certainly `adl_forward_iterator` needn't be a template! And you see why inheriting from things-that-aren't-meant-for-inheritance is a bad idea.)
> I recommend replacing this whole thing with two lines, which can go in their respective .pass.cpp files:
> ```
> static_assert(std::is_class_v<decltype(std::ranges::uninitialized_default_construct)>);
> static_assert(std::is_class_v<decltype(std::ranges::uninitialized_default_construct_n)>);
> ```
> I recommend adding a bonus test case for their SFINAE, as well:
> ```
> struct NotDefaultCtable { NotDefaultCtable(int); };
> static_assert(!std::is_invocable_v<decltype(std::ranges::uninitialized_default_construct), NotDefaultCtable*, NotDefaultCtable*>);
> static_assert(!std::is_invocable_v<decltype(std::ranges::uninitialized_default_construct_n), NotDefaultCtable*, int>);
> ```
> [Using `static_assert(std::is_class_v<...>)`]

Interesting trick! It's true that if it's a class type, then ADL for sure won't trigger, so we'd have the same coverage. However, implementations are technically allowed to use functions (it would presumably be implemented with some sort of attribute) to inhibit ADL, so this would have to be `LIBCPP_STATIC_ASSERT` because `std::is_class_v` would be libc++ specific.

I don't have a strong opinion about this, but I agree this is pretty attractive from a simplicity perspective.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115315