Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rGb9bc3c107c6c: [libc++][ranges] Implement `construct_at` and `destroy{,_at}`.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. | |
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. ;) 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 |
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). |
libcxx/include/__memory/ranges_construct_at.h | ||
---|---|---|
49 | Looks like Clang probably whitelists "everything in std": https://godbolt.org/z/sGx1d1T7o 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. |
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.) |
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. |
- rebase on main and fix the commit history;
- add a TODO to test array support in construct_at.
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.
I don't have a strong preference here. I presume we can postpone this discussion until D114649 lands. |
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.
I miss a lot of _LIBCPP_HIDE_FROM_ABI annotations.