Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++][format] LWG4061: Should std::basic_format_context be default-constructible/copyable/movable? #97251

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Jul 1, 2024

See LWG4061 and P3341R0. Effectively reverts commit 36ce0c3.

libcxx/test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp has a format function that unexpectedly takes the basic_format_context by value, which is made ill-formed by LWG4061. This PR changes the function to take the context by reference.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner July 1, 2024 02:45
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 1, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 1, 2024

@llvm/pr-subscribers-libcxx

Author: A. Jiang (frederick-vs-ja)

Changes

See LWG4061 and P3341R0. Effectively reverts commit 36ce0c3.

libcxx/test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp has a format function that unexpectedly passes the basic_format_context by value, which is made ill-formed by LWG4061. This PR changes the function to pass the context by reference.


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

3 Files Affected:

  • (modified) libcxx/include/__format/format_context.h (+3)
  • (modified) libcxx/test/std/utilities/format/format.formatter/format.context/format.context/ctor.pass.cpp (+14-6)
  • (modified) libcxx/test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp (+1-1)
diff --git a/libcxx/include/__format/format_context.h b/libcxx/include/__format/format_context.h
index 087d4bf289b87..20c07559eae44 100644
--- a/libcxx/include/__format/format_context.h
+++ b/libcxx/include/__format/format_context.h
@@ -131,6 +131,9 @@ class
   _LIBCPP_HIDE_FROM_ABI explicit basic_format_context(_OutIt __out_it, basic_format_args<basic_format_context> __args)
       : __out_it_(std::move(__out_it)), __args_(__args) {}
 #  endif
+
+  basic_format_context(const basic_format_context&)            = delete;
+  basic_format_context& operator=(const basic_format_context&) = delete;
 };
 
 // A specialization for __retarget_buffer
diff --git a/libcxx/test/std/utilities/format/format.formatter/format.context/format.context/ctor.pass.cpp b/libcxx/test/std/utilities/format/format.formatter/format.context/format.context/ctor.pass.cpp
index 40720105060f0..83ece7da73ad3 100644
--- a/libcxx/test/std/utilities/format/format.formatter/format.context/format.context/ctor.pass.cpp
+++ b/libcxx/test/std/utilities/format/format.formatter/format.context/format.context/ctor.pass.cpp
@@ -112,19 +112,27 @@ void test() {
 #endif
 }
 
+// The default constructor is suppressed by the deleted copy operations.
+// The move operations are implicitly deleted due to the deleted copy operations.
+
 // std::back_insert_iterator<std::string>, copyable
-static_assert(std::is_copy_constructible_v<std::basic_format_context<std::back_insert_iterator<std::string>, char>>);
-static_assert(std::is_copy_assignable_v<std::basic_format_context<std::back_insert_iterator<std::string>, char>>);
+static_assert(
+    !std::is_default_constructible_v<std::basic_format_context<std::back_insert_iterator<std::string>, char>>);
+
+static_assert(!std::is_copy_constructible_v<std::basic_format_context<std::back_insert_iterator<std::string>, char>>);
+static_assert(!std::is_copy_assignable_v<std::basic_format_context<std::back_insert_iterator<std::string>, char>>);
 
-static_assert(std::is_move_constructible_v<std::basic_format_context<std::back_insert_iterator<std::string>, char>>);
-static_assert(std::is_move_assignable_v<std::basic_format_context<std::back_insert_iterator<std::string>, char>>);
+static_assert(!std::is_move_constructible_v<std::basic_format_context<std::back_insert_iterator<std::string>, char>>);
+static_assert(!std::is_move_assignable_v<std::basic_format_context<std::back_insert_iterator<std::string>, char>>);
 
 // cpp20_output_iterator, move only
+static_assert(!std::is_default_constructible_v<std::basic_format_context<cpp20_output_iterator<int*>, char>>);
+
 static_assert(!std::is_copy_constructible_v<std::basic_format_context<cpp20_output_iterator<int*>, char>>);
 static_assert(!std::is_copy_assignable_v<std::basic_format_context<cpp20_output_iterator<int*>, char>>);
 
-static_assert(std::is_move_constructible_v<std::basic_format_context<cpp20_output_iterator<int*>, char>>);
-static_assert(std::is_move_assignable_v<std::basic_format_context<cpp20_output_iterator<int*>, char>>);
+static_assert(!std::is_move_constructible_v<std::basic_format_context<cpp20_output_iterator<int*>, char>>);
+static_assert(!std::is_move_assignable_v<std::basic_format_context<cpp20_output_iterator<int*>, char>>);
 
 int main(int, char**) {
   test<std::back_insert_iterator<std::basic_string<char>>, char>();
diff --git a/libcxx/test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp b/libcxx/test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp
index 127f6546ccbe7..5f248f88582d0 100644
--- a/libcxx/test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp
+++ b/libcxx/test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp
@@ -25,7 +25,7 @@ struct X : std::variant<X*> {
 
 template <>
 struct std::formatter<X, char> : std::formatter<std::string, char> {
-  static constexpr auto format(const X& x, auto ctx) {
+  static constexpr auto format(const X& x, auto& ctx) {
     if (!x.p)
       return ctx.out();
     auto m = [&](const X* t) { return std::format_to(ctx.out(), "{}", *t); };

@mordante mordante self-assigned this Jul 4, 2024
Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM!

(I'll make sure this is updated on the status page once we updated it for St. Louis.)

@mordante
Copy link
Member

mordante commented Jul 7, 2024

@frederick-vs-ja do you have commit access for this repository?

@mordante
Copy link
Member

mordante commented Jul 8, 2024

I've just committed #97951 can you rebase your patch on main and update the status page for this patch.

Effectively reverts commit 36ce0c3.
@frederick-vs-ja
Copy link
Contributor Author

@frederick-vs-ja do you have commit access for this repository?

No, I don't...

I've just committed #97951 can you rebase your patch on main and update the status page for this patch.

Done.

@mordante
Copy link
Member

mordante commented Jul 9, 2024

@frederick-vs-ja do you have commit access for this repository?

No, I don't...

I noticed you have quite a number of patches merge feel free to request commit access if you want.

I've just committed #97951 can you rebase your patch on main and update the status page for this patch.

Done.

Thanks.

@mordante mordante merged commit ca055bb into llvm:main Jul 9, 2024
54 checks passed
@frederick-vs-ja frederick-vs-ja deleted the lwg-4061 branch July 9, 2024 10:29
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
…lt-constructible/copyable/movable? (llvm#97251)

See [LWG4061](https://cplusplus.github.io/LWG/issue4061) and
[P3341R0](https://wg21.link/p3341r0). Effectively reverts commit
36ce0c3.


`libcxx/test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp`
has a `format` function that unexpectedly takes the
`basic_format_context` by value, which is made ill-formed by LWG4061.
This PR changes the function to take the context by reference.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants