[libc++] Fix missing declarations of uses_allocator_construction_args #67044

phyBrackets · 2023-09-21T17:26:39Z

We were not declaring __uses_allocator_construction_args helper
functions, leading to several valid uses failing to compile. This
patch solves the problem by moving these helper functions into a
struct, which also reduces the amount of redundant SFINAE we need
to perform since most overloads are checking for a cv-qualfied pair.

Fixes #66714

llvmbot · 2023-09-21T17:27:44Z

@llvm/pr-subscribers-libcxx

Changes

Fixing #66714

Full diff: https://github.com/llvm/llvm-project/pull/67044.diff

1 Files Affected:

(modified) libcxx/include/__memory/uses_allocator_construction.h (+32-6)

diff --git a/libcxx/include/__memory/uses_allocator_construction.h b/libcxx/include/__memory/uses_allocator_construction.h
index a2e4f6e26f4b3af..b4e3b54bf39b8f7 100644
--- a/libcxx/include/__memory/uses_allocator_construction.h
+++ b/libcxx/include/__memory/uses_allocator_construction.h
@@ -53,6 +53,32 @@ __uses_allocator_construction_args(const _Alloc& __alloc, _Args&&... __args) noe
 }
 
 template <class _Pair, class _Alloc, class _Tuple1, class _Tuple2, __enable_if_t<__is_std_pair<_Pair>, int> = 0>
+_LIBCPP_HIDE_FROM_ABI constexpr auto
+__uses_allocator_construction_args(const _Alloc& __a, piecewise_construct_t, _Tuple1&& __x, _Tuple2&& __y) noexcept;
+
+template <class _Pair, class _Alloc, __enable_if_t<__is_std_pair<_Pair>, int> = 0>
+_LIBCPP_HIDE_FROM_ABI constexpr auto __uses_allocator_construction_args(const _Alloc&) noexcept;
+
+template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int> = 0>
+_LIBCPP_HIDE_FROM_ABI constexpr auto __uses_allocator_construction_args(const _Alloc&, _Up&&, _Vp&&) noexcept;
+
+template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int> = 0>
+_LIBCPP_HIDE_FROM_ABI constexpr auto
+__uses_allocator_construction_args(const _Alloc& __alloc, pair<_Up, _Vp>& __pair) noexcept;
+
+template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int> = 0>
+_LIBCPP_HIDE_FROM_ABI constexpr auto
+__uses_allocator_construction_args(const _Alloc& __alloc, const pair<_Up, _Vp>& __pair) noexcept;
+
+template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int> = 0>
+_LIBCPP_HIDE_FROM_ABI constexpr auto
+__uses_allocator_construction_args(const _Alloc& __alloc, pair<_Up, _Vp>&& __pair) noexcept;
+
+template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int> = 0>
+_LIBCPP_HIDE_FROM_ABI constexpr auto
+__uses_allocator_construction_args(const _Alloc& __alloc, const pair<_Up, _Vp>&& __pair) noexcept;
+
+template <class _Pair, class _Alloc, class _Tuple1, class _Tuple2, __enable_if_t<__is_std_pair<_Pair>, int>>
 _LIBCPP_HIDE_FROM_ABI constexpr auto __uses_allocator_construction_args(
     const _Alloc& __alloc, piecewise_construct_t, _Tuple1&& __x, _Tuple2&& __y) noexcept {
   return std::make_tuple(
@@ -71,12 +97,12 @@ _LIBCPP_HIDE_FROM_ABI constexpr auto __uses_allocator_construction_args(
           std::forward<_Tuple2>(__y)));
 }
 
-template <class _Pair, class _Alloc, __enable_if_t<__is_std_pair<_Pair>, int> = 0>
+template <class _Pair, class _Alloc, __enable_if_t<__is_std_pair<_Pair>, int>>
 _LIBCPP_HIDE_FROM_ABI constexpr auto __uses_allocator_construction_args(const _Alloc& __alloc) noexcept {
   return std::__uses_allocator_construction_args<_Pair>(__alloc, piecewise_construct, tuple<>{}, tuple<>{});
 }
 
-template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int> = 0>
+template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int>>
 _LIBCPP_HIDE_FROM_ABI constexpr auto
 __uses_allocator_construction_args(const _Alloc& __alloc, _Up&& __u, _Vp&& __v) noexcept {
   return std::__uses_allocator_construction_args<_Pair>(
@@ -87,7 +113,7 @@ __uses_allocator_construction_args(const _Alloc& __alloc, _Up&& __u, _Vp&& __v)
 }
 
 #  if _LIBCPP_STD_VER >= 23
-template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int> = 0>
+template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int>>
 _LIBCPP_HIDE_FROM_ABI constexpr auto
 __uses_allocator_construction_args(const _Alloc& __alloc, pair<_Up, _Vp>& __pair) noexcept {
   return std::__uses_allocator_construction_args<_Pair>(
@@ -95,14 +121,14 @@ __uses_allocator_construction_args(const _Alloc& __alloc, pair<_Up, _Vp>& __pair
 }
 #  endif
 
-template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int> = 0>
+template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int>>
 _LIBCPP_HIDE_FROM_ABI constexpr auto
 __uses_allocator_construction_args(const _Alloc& __alloc, const pair<_Up, _Vp>& __pair) noexcept {
   return std::__uses_allocator_construction_args<_Pair>(
       __alloc, piecewise_construct, std::forward_as_tuple(__pair.first), std::forward_as_tuple(__pair.second));
 }
 
-template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int> = 0>
+template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int>>
 _LIBCPP_HIDE_FROM_ABI constexpr auto
 __uses_allocator_construction_args(const _Alloc& __alloc, pair<_Up, _Vp>&& __pair) noexcept {
   return std::__uses_allocator_construction_args<_Pair>(
@@ -113,7 +139,7 @@ __uses_allocator_construction_args(const _Alloc& __alloc, pair<_Up, _Vp>&& __pai
 }
 
 #  if _LIBCPP_STD_VER >= 23
-template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int> = 0>
+template <class _Pair, class _Alloc, class _Up, class _Vp, __enable_if_t<__is_std_pair<_Pair>, int>>
 _LIBCPP_HIDE_FROM_ABI constexpr auto
 __uses_allocator_construction_args(const _Alloc& __alloc, const pair<_Up, _Vp>&& __pair) noexcept {
   return std::__uses_allocator_construction_args<_Pair>(

philnik777 · 2023-09-21T18:19:47Z

This is definitely not NFC and there should be tests added.

phyBrackets · 2023-09-22T04:51:50Z

Hi @philnik777 , thanks for the comment! I am kinda unsure about the current way of updating the tests is the best, have a look.

github-actions · 2023-09-22T12:33:39Z

✅ With the latest revision this PR passed the C/C++ code formatter.

phyBrackets · 2023-09-23T14:12:48Z

Not really sure why, test fail over modular build?

ldionne

I think this looks pretty good to me, but I am struggling to figure whether there's other similar cases that would be problematic and for which we should add test coverage.

@philnik777 can you take a look since you wrote this in the first place?

.../memory/allocator.uses/allocator.uses.construction/uses_allocator_construction_args.pass.cpp

libcxx/test/std/utilities/memory/allocator.uses/allocator.uses.construction/common.h

ldionne · 2023-09-27T16:17:03Z

Also, this should probably be rebased on top of #66939 once it lands -- I think it'll be easier to do that than the other way around.

EricWF · 2023-10-09T18:48:51Z

LGTM, but the bots need to get green and this change needs to be updated to merge cleanly.

phyBrackets · 2023-10-10T19:31:25Z

Thanks @ldionne and @EricWF !

philnik777

This looks to me like we're adding a lot more forward declarations than tests, which makes me suspect that this is fixing bugs which we don't add regression tests for. Please make sure we test all the overloads (especially differently qualified pair overloads).

.../memory/allocator.uses/allocator.uses.construction/uses_allocator_construction_args.pass.cpp

phyBrackets · 2023-10-16T17:41:50Z

@philnik777 does this looks good to you? Other than format issue, I am not sure what's the actual issue even running git clang-format on the commit , seems like CI doesn't like the formatting and so complaining.

philnik777 · 2023-10-17T09:42:07Z

It looks like you have some non-ASCII characters in your source. Did you copy the synopsis from eel.is/c++draft/? In that case they're probably in there.

phyBrackets · 2023-10-17T10:10:03Z

No, I don't think so 🤔. Although I have tried to look at any non-ASCII character in case, using regex but didn't find any.

philnik777 · 2023-10-17T14:20:30Z

Oh sorry. I misread the output. It looks like the clang-format versions of the formatting checker and our generated output runner are different. Just add libcxx/test/std/utilities/memory/allocator.uses/allocator.uses.construction/uses_allocator_construction_args.pass.cpp to the ignore_format.txt and run the CI again.

phyBrackets · 2023-10-17T19:15:13Z

Thank you @philnik777 . Thoughts on the updated patch?

philnik777 · 2023-10-21T11:31:22Z

Could you ping me again when the CI is green?

ldionne · 2023-11-01T14:52:46Z

It looks like this is only failing in C++20 mode now, hopefully this should be pretty easy to fix?

ldionne · 2023-11-23T23:08:34Z

Ping. I'd like to get this review finished and merged. If you have no time to work on it, please let us know and we'll find someone to pick it up, but we'd like to avoid accumulating too many outstanding reviews cause it makes the operation of the project harder.

mordante · 2023-12-24T11:56:35Z

@phyBrackets are you still interested in finishing this patch?

ldionne · 2024-01-08T18:33:16Z

I'll let @philnik777 finish the review on this one, don't let my "request for changes" block a merge.

phyBrackets · 2024-01-22T12:40:46Z

Hi @philnik777 , Can you please review this? I am not actually sure what causing CI to fails.

phyBrackets requested a review from a team as a code owner September 21, 2023 17:26

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 21, 2023

ldionne requested a review from huixie90 September 21, 2023 19:53

phyBrackets changed the title ~~[Libcxx][NFC] fixing missing forward declarations of uses_allocator_construction_args~~ [libc++] fixing missing forward declarations of uses_allocator_construction_args Sep 22, 2023

ldionne requested changes Sep 27, 2023

View reviewed changes

.../memory/allocator.uses/allocator.uses.construction/uses_allocator_construction_args.pass.cpp Show resolved Hide resolved

libcxx/test/std/utilities/memory/allocator.uses/allocator.uses.construction/common.h Outdated Show resolved Hide resolved

ldionne assigned philnik777 Sep 27, 2023

philnik777 requested changes Oct 10, 2023

View reviewed changes

.../memory/allocator.uses/allocator.uses.construction/uses_allocator_construction_args.pass.cpp Outdated Show resolved Hide resolved

phyBrackets requested a review from Endilll as a code owner December 28, 2023 07:01

Endilll removed their request for review December 29, 2023 11:07

phyBrackets and others added 2 commits July 31, 2024 16:17

forward declarations for __uses_allocator_construction_args

2b5ba4f

Define everything in a struct

a12c1f4

ldionne changed the title ~~[libc++] fixing missing forward declarations of uses_allocator_construction_args~~ [libc++] Fix missing declarations of uses_allocator_construction_args Jul 31, 2024

ldionne approved these changes Jul 31, 2024

View reviewed changes

ldionne merged commit beecf2c into llvm:main Aug 1, 2024
54 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] Fix missing declarations of uses_allocator_construction_args #67044

[libc++] Fix missing declarations of uses_allocator_construction_args #67044

phyBrackets commented Sep 21, 2023 •

edited by ldionne

Loading

llvmbot commented Sep 21, 2023

philnik777 commented Sep 21, 2023

phyBrackets commented Sep 22, 2023

github-actions bot commented Sep 22, 2023 •

edited

Loading

phyBrackets commented Sep 23, 2023

ldionne left a comment

ldionne commented Sep 27, 2023

EricWF commented Oct 9, 2023

phyBrackets commented Oct 10, 2023

philnik777 left a comment

phyBrackets commented Oct 16, 2023

philnik777 commented Oct 17, 2023

phyBrackets commented Oct 17, 2023

philnik777 commented Oct 17, 2023

phyBrackets commented Oct 17, 2023

philnik777 commented Oct 21, 2023

ldionne commented Nov 1, 2023

ldionne commented Nov 23, 2023

mordante commented Dec 24, 2023

ldionne commented Jan 8, 2024

phyBrackets commented Jan 22, 2024

[libc++] Fix missing declarations of uses_allocator_construction_args #67044

[libc++] Fix missing declarations of uses_allocator_construction_args #67044

Conversation

phyBrackets commented Sep 21, 2023 • edited by ldionne Loading

llvmbot commented Sep 21, 2023

philnik777 commented Sep 21, 2023

phyBrackets commented Sep 22, 2023

github-actions bot commented Sep 22, 2023 • edited Loading

phyBrackets commented Sep 23, 2023

ldionne left a comment

Choose a reason for hiding this comment

ldionne commented Sep 27, 2023

EricWF commented Oct 9, 2023

phyBrackets commented Oct 10, 2023

philnik777 left a comment

Choose a reason for hiding this comment

phyBrackets commented Oct 16, 2023

philnik777 commented Oct 17, 2023

phyBrackets commented Oct 17, 2023

philnik777 commented Oct 17, 2023

phyBrackets commented Oct 17, 2023

philnik777 commented Oct 21, 2023

ldionne commented Nov 1, 2023

ldionne commented Nov 23, 2023

mordante commented Dec 24, 2023

ldionne commented Jan 8, 2024

phyBrackets commented Jan 22, 2024

phyBrackets commented Sep 21, 2023 •

edited by ldionne

Loading

github-actions bot commented Sep 22, 2023 •

edited

Loading