Page MenuHomePhabricator

[libc++][ranges] Implement `construct_at` and `destroy{,_at}`.
ClosedPublic

Authored by var-const on Dec 20 2021, 9:06 PM.

Details

Diff Detail

Event Timeline

var-const created this revision.Dec 20 2021, 9:06 PM
var-const requested review of this revision.Dec 20 2021, 9:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2021, 9:06 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const updated this revision to Diff 395586.Dec 20 2021, 9:08 PM

Use the actual revision number on status pages.

LprettyGTM; just nits.

libcxx/include/__memory/ranges_construct_at.h
49

As always, I'd feel better if this were

return ::new (_VSTD::__voidify(*__location)) _Tp(_VSTD::forward<_Args>(__args)...);

(for obvious-correctness, simplicity of implementation, and efficiency-at--O0) but I suppose the extra-indirection way has the minor benefit that if we ever need to do any special magic to make construct_at constexpr-friendly, we'll only need to do it in one place? (For better or worse, I guess. If it later turned out that that magic was required to happen only in construct_at and not in ranges::construct_at, then we'd have to go back to the simple ::new-expression here anyway.)

58

You've got an extra blank line here (and line 79); and please compare to other CPO definitions to make sure you're consistent with the indentation.
I recommend looking on the right-hand side here: https://reviews.llvm.org/D115607#change-mUEJDUqE3FiU

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.construct/ranges_construct_at.pass.cpp
68–77

Let's make this

struct Foo {
    int a_;
    int b_;
    explicit Foo(int a, int b) : a_(b), b_(a) {}
    Foo(std::initializer_list<int>);
    void operator&() const = delete;
    void operator,(auto&&) const = delete;
};
~~~
Foo f(0, 0);  // or auto f = Foo(0, 0);
Foo *result = std::ranges::construct_at(std::addressof(f), 1, 2);
assert(result == std::addressof(f));
assert(result.a == 2);
assert(result.b == 1);

The major thing here is that I added initializer_list to make sure the library is correctly constructing with Foo(1,2) instead of Foo{1,2}. Then I threw in the deleted & and , overloads as an afterthought; I'm not really attached to them, but they seemed low-cost enough to justify their low benefit.

110–112

I smell clang-format's meddling in this weird comment-inside-the-parens style. ;)
The comments are a bit confusing, especially coupled with the use of nullptr — we do check for nullptr inside construct_at, but that part is specifically not SFINAE-friendly. What I'd rather see here is something like

constexpr bool can_construct_at(auto&&... args)
  requires requires { std::ranges::construct_at(decltype(args)(args)...); }
  { return true; }

constexpr bool can_construct_at(auto&&... args)
  { return false; }

static_assert( can_construct_at((Foo*)nullptr, 1, 2));
static_assert(!can_construct_at((Foo*)nullptr, 1));
static_assert(!can_construct_at((Foo*)nullptr, 1, 2, 3));
static_assert(!can_construct_at(nullptr, 1, 2));
static_assert(!can_construct_at((int*)nullptr, 1, 2));
static_assert(!can_construct_at(contiguous_iterator<Foo*>(), 1, 2));
127

I'd like to see some tests with array types, if that's supposed to work; or otherwise,

static_assert(!can_construct_at((int(*)[])nullptr));
static_assert(!can_construct_at((int(*)[2])nullptr));
static_assert(!can_construct_at((int(*)[2])nullptr, 1, 2));

// and function types while we're at it
static_assert(!can_construct_at((int(*)())nullptr));
static_assert(!can_construct_at((int(*)())nullptr, nullptr));
libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/ranges_destroy_at.pass.cpp
52

Passing a polymorphic type by value should be a red flag. Let's make this
void operator&() const = delete;
and remove it from the derived class (because the derived class will inherit this one).

Mordante added inline comments.
libcxx/include/__memory/ranges_construct_at.h
43

I miss a lot of _LIBCPP_HIDE_FROM_ABI annotations.

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/ranges_destroy.pass.cpp
219

Please add a link to D114903, which implements the missing feature.

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/ranges_destroy_at.pass.cpp
155

Please add a link to D114903, which implements the missing feature.

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/ranges_destroy_n.pass.cpp
141

Please add a link to D114903, which implements the missing feature.

Leaving comments but I won't mark as "needs changes" to avoid blocking you. If you implement my suggestions and get ✅ from other libc++ reviewers, 🚢 it.

libcxx/include/__memory/ranges_construct_at.h
49

It's actually the other way around, if you use ::new (location) then it won't be constexpr-friendly, right? std::construct_at was introduced exactly to make this sort of pattern constexpr friendly, unless I'm having a giant brain fart.

This seems to confirm what I'm saying: https://godbolt.org/z/9jb7jx4q3

TLDR: AFAICT, this is the only correct way to implement the function without having to change Clang (and possibly the other compilers we support).

ldionne accepted this revision as: ldionne.Dec 21 2021, 11:50 AM
libcxx/include/__memory/ranges_construct_at.h
49

Looks like Clang probably whitelists "everything in std": https://godbolt.org/z/sGx1d1T7o
GCC must do something more narrowly tailored: I see their own std::ranges::construct_at is constexpr-friendly, but I haven't been able to craft one myself.

Anyway, I had not realized that std::ranges::construct_at was explicitly mentioned in expr.const/6 alongside std::construct_at; therefore it's already supposed to have the same constexpr magic as std::construct_at. So okay, I still think I'd rather see ::new in both places, but if this way is projected to help us jump through GCC's hoops better, then I certainly won't block over it.

var-const updated this revision to Diff 395790.Dec 21 2021, 8:12 PM
var-const marked 11 inline comments as done.

Address feedback.

libcxx/include/__memory/ranges_construct_at.h
43

Thanks for spotting this! It also affects my previous patches in this series (I'll do a fix patch once this series lands)

49

Keeping as is given @ldionne's comment.

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.construct/ranges_construct_at.pass.cpp
68–77

Done, thanks!

110–112

Thanks! This definitely feels more comprehensive than the previous state. I've also backported this to the non-ranges test (from which I copied the previous state).

127

Added the test for function types.

The situation with arrays is interesting. They don't work due to how the return type is currently specified (see https://godbolt.org/z/sT8xM7PvP), but there's no requirement that arrays should be SFINAE'd away. Also, there's a recently-updated proposal to support arrays: LWG 3436.

So we can a) make an extension to SFINAE away array types, then make it conditional in C++23 (or just remove it altogether) or b) do nothing and allow such calls to fail with a hard error. What do you think?

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/ranges_destroy.pass.cpp
219

Thanks for the link! Added to the new files and also to tests of the old versions of these algorithms.

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/ranges_destroy_at.pass.cpp
52

Changed here and in tests for the non-ranges version of destroy_at.

libcxx/test/std/utilities/memory/specialized.algorithms/specialized.construct/ranges_construct_at.pass.cpp
127

Personally, I'd just implement the proposed resolution for LWG3436 (in C++20-and-later). It's a good fix for a bug in the Standard, and it's always useful to get implementation experience with this kind of thing ASAP. Also, we don't want people to start relying on the wrong behavior and then change it out from under them in C++23. Also, once we've implemented the correct behavior, there's no reason to limit it to C++23-and-later when it could apply just as well to C++20. (We'd still have to decide what to do in C++20, because what was published is not implementable.)

ldionne accepted this revision.Dec 29 2021, 6:02 AM
ldionne added inline comments.
libcxx/test/std/utilities/memory/specialized.algorithms/specialized.construct/ranges_construct_at.pass.cpp
127

See https://reviews.llvm.org/D114649 for rectifying the solution for array types.

This revision is now accepted and ready to land.Dec 29 2021, 6:02 AM
var-const updated this revision to Diff 397002.Jan 2 2022, 11:01 PM
var-const marked 2 inline comments as not done.
  • rebase on main and fix the commit history;
  • add a TODO to test array support in construct_at.
var-const added inline comments.Jan 2 2022, 11:02 PM
libcxx/test/std/utilities/memory/specialized.algorithms/specialized.construct/ranges_construct_at.pass.cpp
127

Given that there's an in-progress patch addressing LWG 3436, I'm inclined to leave as-is with a TODO to test array types once the patch lands.

(We'd still have to decide what to do in C++20, because what was published is not implementable.)

I don't have a strong preference here. I presume we can postpone this discussion until D114649 lands.

var-const updated this revision to Diff 399173.Jan 11 2022, 7:10 PM

Fix CI (part 3).

var-const updated this revision to Diff 399474.Jan 12 2022, 2:33 PM

Replace the use of concepts in the construct_at test with SFINAE. While
construct_at requires C++20, older compiler versions might not implement
concepts in that mode.