[libcxx-commits] [PATCH] D115315: [libc++][ranges] Implement ranges::uninitialized_default_construct{, _n}.
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Dec 8 20:01:00 PST 2021
var-const added a comment.
Note: I have refactored the repetitive initialization code into a helper `Buffer` class that I hope can be reused across similar tests.
================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:8
+//
+//===----------------------------------------------------------------------===//
+
----------------
ldionne wrote:
> 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.
I'm slightly in favor of keeping these in a single file because they're closely related and I personally find it easier to read when related things are all in one place. I don't have a strong objection to separating these, though.
What I can do is to keep everything in a single file for the initial implementation and then have an `NFC` commit to split them. I hope at that point it would be clearer which approach is more readable here, and we can decide whether to merge or revert that patch.
================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:32
+
+struct __uninitialized_default_construct_fn final : private __function_like {
+
----------------
ldionne wrote:
> 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".
I think the idea here is to make sure no users can end up accidentally depending on any properties that aren't explicitly guaranteed by the Standard (and yes, their code would be in error in that case, but it's still a possible problem that would require somebody to spend time fixing it, and we can prevent that problem from happening in the implementation). It does go beyond what is strictly necessary for conformance, but arguably it can result in slightly better experience for the users. Moreover, removing `__function_like` in the future should be a non-breaking change (I don't think ABI is a concern here, though I can easily be wrong on this), whereas going the other way round could realistically break somebody.
Also, we already have precedent in some of the iterator algorithms (e.g. `advance` and `next`). This isn't a particularly strong argument because there are very few instances, so changing them is quite simple. Basically, we would just need to make sure we are consistent.
================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:51
+
+};
+
----------------
Quuxplusone wrote:
> Line 47: please `T&& t`, not `T &&t`
> Lines 39, 46, 64: please indent the `requires`-clause two spaces
Done, thanks!
================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:53-54
+
+inline constexpr auto uninitialized_default_construct =
+ __uninitialized_default_construct_fn(__function_like::__tag());
+
----------------
Quuxplusone wrote:
> Here and line 73, please `inline namespace __cpo { ~~~ }`. (See https://quuxplusone.github.io/blog/2021/12/07/namespace-cpo/ for my rationale, which boils down to "consistency.")
Thanks! Very helpful article.
================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:68
+ using _ValueType = remove_reference_t<iter_reference_t<_ForwardIterator>>;
+ return _VSTD::__uninitialized_default_construct_n<_ValueType>(__first, __n);
+ }
----------------
Quuxplusone wrote:
> This is actually specced as
> ```
> return uninitialized_default_construct(counted_iterator(first, n), default_sentinel).base();
> ```
> but is the difference observable? I can't think of a reason it would be, so I'm fine with this as you've written it.
Sorry, I should have called it out in my comments. Yes, I did this deliberately because I couldn't think of a case where the two forms wouldn't be equivalent, and delegating directly to the internal helper `_n` function seemed simpler than creating a wrapped iterator then delegating to the other helper.
================
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:
> ldionne wrote:
> > 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)`.
> (Argh, I keep forgetting to reply to things on the first go-round. Sorry.)
> IIUC, STL's comment there indicates that we should add a test case for const iterators, e.g.
> ```
> auto cit = forward_iterator<const Counted*>(as_array);
> auto r = std::ranges::subrange(cit, sentinel_wrapper<forward_iterator<const Counted*>>(cit + 2));
> std::ranges::uninitialized_default_construct(r.begin(), r.end());
> assert(Counted::current_objects == 2);
> assert(Counted::total_objects == 2);
> ```
> That's missing test coverage today, but AFAIK it's not an argument against the simple cast to `(void*)`. C-style casts are allowed to cast away constness.
Added the test.
It looks like until C++20, the Standard defined `voidify` as performing a simple `static_cast<void*>` cast, but that would fail if the given type is cv-qualified because `static_cast` can't cast away cv qualifiers. The new definition works around it by doing two casts, a `static_cast` followed by a `const_cast`. However, it does seem like a simple C-style cast can achieve the same effect. I checked that the C-style cast succeeds when used with const iterators whereas the old `static_cast<void*>` approach fails as expected.
I still feel like we should have a dedicated `__voidify` function (that can use the C-style cast). For one, it would allow defining (and documenting) our approach in one place. It would also follow the way things are laid out in the Standard more closely, which I think is a nice bonus.
================
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
----------------
ldionne wrote:
> Need to add `libcpp-has-no-incomplete-ranges`.
Thanks!
================
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
+
----------------
ldionne wrote:
> We don't support it anymore, none of our tests mention this anymore unless I'm mistaken.
Done. I think I copied it from an unsubmitted patch which presumably was written at a time when GCC 10 was still supported.
================
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] = {};
----------------
ldionne wrote:
> 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
> }
> ```
>
Done.
================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct.pass.cpp:61-71
+ alignas(Counted) char buffer[sizeof(Counted) * 1] = {};
+ auto* as_array = reinterpret_cast<Counted*>(buffer);
+
+ ranges::uninitialized_default_construct(as_array, as_array);
+ assert(Counted::current_objects == 0);
+ assert(Counted::total_objects == 0);
+
----------------
Quuxplusone wrote:
> Let's also do some non-common ranges. I suggest these three tests in place of lines 68–71:
> ```
> std::ranges::uninitialized_default_construct(std::ranges::empty<Counted>); // [basically the same as your 68–69, but gives nullptr for begin and end instead of as_array]
> assert(Counted::current_objects == 0);
> assert(Counted::total_objects == 0);
>
> auto it = forward_iterator<Counted*>(as_array);
> auto r = std::ranges::subrange(it, sentinel_wrapper<forward_iterator<Counted*>>(it));
> std::ranges::uninitialized_default_construct(r.begin(), r.end());
> assert(Counted::current_objects == 0);
> assert(Counted::total_objects == 0);
>
> std::ranges::uninitialized_default_construct(r);
> assert(Counted::current_objects == 0);
> assert(Counted::total_objects == 0);
> ```
>
> Ditto throughout: please make sure to test non-common ranges, because that's really the major feature you're actually adding here (e.g. that's why you had to modify the non-ranges version of the algorithm).
Added `counted_iterator`, `views::counted` and `reverse_view`. If you have any other suggestions, please let me know.
================
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);
+
----------------
ldionne wrote:
> 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.
Thank you for explaining your rationale. The main reason I generally prefer using `auto` when doing casts is that `auto` can be taken to mean "use the exact same type as returned by the expression" (omitting nuances about constness and references for the purposes of this discussion). In other words, `auto` guarantees that no implicit conversion can take place, which could be relevant in a case like:
```
struct Base { ... };
struct Derived : Base { ... };
Base* p = reinterpret_cast<Derived*>(buffer); // Implicitly converted from `Derived` to `Base`
auto* p = reinterpret_cast<Derived*>(buffer); // Reader can be sure no implicit conversion can take place
```
While this isn't relevant here, this usage of `auto` arguably simplifies things for the reader by removing one case to watch out for (if used consistently).
Having said that, I see the value of your point as well. I was somewhat on the fence, so I decided to use consistency as a tie-breaker. A simple `grep` shows a single case of using `auto` with casts in the existing code; most existing casts specify the type directly. For this reason, I decided to go with your suggestion; after all, the benefits of either approach are mostly relevant only if they are followed consistently.
EDIT: after writing this, I refactored this code so that it became a member function and this is no longer applicable.
================
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);
----------------
ldionne wrote:
> 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.
Thanks! Removed it here and in `uninitialized_default_construct{,_n}.pass.cpp`.
================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct_n.pass.cpp:27
+
+namespace ranges = std::ranges;
+
----------------
Quuxplusone wrote:
> I forgot to mention, I'm not a big fan of this alias (I'd rather see `std::ranges::` everywhere), but if you //must// pick a name for the alias, then this is 100% the best name. It prevents me from `git grep -l std::ranges::foo`, but at least I can still `git grep -l ranges::foo` if I suspect this trick has been played. :)
Removed the alias. I took it from some existing code for consistency, but I don't have a strong preference.
================
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:
> ldionne wrote:
> > 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.
> > implementations are technically allowed to use functions
>
> Sure, //technically//, but until literally anyone does even a //proof of concept// patch to literally any compiler in the world to permit that, I think we can discount the possibility. ;) (This is closely adjacent to my logic against `__function_like`, btw. Everyone knows that niebloids are objects-not-functions; there's no engineering benefit gained by pretending things aren't as they are, and meanwhile there //are// engineering costs.)
This approach is definitely very attractive due to its simplicity. These ADL tests are verbose, apply to every single range algorithm and can't really be simplified much (AFAICT). On the other hand, the current approach has the advantage of testing the effects whereas the new approach tests the implementation.
All in all, I'm inclined to go with the simpler approach because the boilerplate savings it provides are very significant. Thanks for suggesting this!
As you suggested, I moved these checks to the existing test files. I have also used `LIBCPP_STATIC_ASSERT`, though I don't feel super strongly about it (from a practical point of view, I presume it's true that no existing implementation uses or has any particular reason to use a different implementation method).
I've also added a TODO to rewrite the existing tests using the new approach.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115315/new/
https://reviews.llvm.org/D115315
More information about the libcxx-commits
mailing list