Skip to content

[libc++][syncbuf] Implements LWG3253. #99778

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

Merged
merged 1 commit into from
Aug 30, 2024
Merged

Conversation

mordante
Copy link
Member

@mordante mordante commented Jul 20, 2024

Note the tests already tested this behaviour and the wording change is NFC.

Implements

  • LWG3253 basic_syncbuf::basic_syncbuf() should not be explicit

Closes #100264

@mordante mordante requested a review from a team as a code owner July 20, 2024 19:21
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 20, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 20, 2024

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

Note the tests already tested this behaviour and the wording change is NFC.

Implements

  • LWG3253 basic_syncbuf::basic_syncbuf() should not be explicit

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

2 Files Affected:

  • (modified) libcxx/docs/Status/Cxx20Issues.csv (+1-1)
  • (modified) libcxx/include/syncstream (+7-2)
diff --git a/libcxx/docs/Status/Cxx20Issues.csv b/libcxx/docs/Status/Cxx20Issues.csv
index 1a40a4472a405..2dc1f45c5f02d 100644
--- a/libcxx/docs/Status/Cxx20Issues.csv
+++ b/libcxx/docs/Status/Cxx20Issues.csv
@@ -172,7 +172,7 @@
 "`3221 <https://wg21.link/LWG3221>`__","Result of ``year_month``\  arithmetic with ``months``\  is ambiguous","Belfast","|Complete|","8.0"
 "`3235 <https://wg21.link/LWG3235>`__","``parse``\  manipulator without abbreviation is not callable","Belfast","",""
 "`3246 <https://wg21.link/LWG3246>`__","What are the constraints on the template parameter of ``basic_format_arg``\ ?","Belfast","","","|format|"
-"`3253 <https://wg21.link/LWG3253>`__","``basic_syncbuf::basic_syncbuf()``\  should not be explicit","Belfast","",""
+"`3253 <https://wg21.link/LWG3253>`__","``basic_syncbuf::basic_syncbuf()``\  should not be explicit","Belfast","|Complete|","20.0"
 "`3245 <https://wg21.link/LWG3245>`__","Unnecessary restriction on ``'%p'``\  parse specifier","Belfast","","","|chrono|"
 "`3244 <https://wg21.link/LWG3244>`__","Constraints for ``Source``\  in |sect|\ [fs.path.req] insufficiently constrainty","Belfast","",""
 "`3241 <https://wg21.link/LWG3241>`__","``chrono-spec``\  grammar ambiguity in |sect|\ [time.format]","Belfast","|Complete|","16.0","|chrono| |format|"
diff --git a/libcxx/include/syncstream b/libcxx/include/syncstream
index e6f35b6f428ed..a0617f4acf5b6 100644
--- a/libcxx/include/syncstream
+++ b/libcxx/include/syncstream
@@ -46,7 +46,9 @@ namespace std {
         using streambuf_type = basic_streambuf<charT, traits>;
 
         // [syncstream.syncbuf.cons], construction and destruction
-        explicit basic_syncbuf(streambuf_type* obuf = nullptr)
+        basic_syncbuf()
+          : basic_syncbuf(nullptr) {}
+        explicit basic_syncbuf(streambuf_type* obuf)
           : basic_syncbuf(obuf, Allocator()) {}
         basic_syncbuf(streambuf_type*, const Allocator&);
         basic_syncbuf(basic_syncbuf&&);
@@ -253,7 +255,10 @@ public:
 
   // [syncstream.syncbuf.cons], construction and destruction
 
-  _LIBCPP_HIDE_FROM_ABI explicit basic_syncbuf(streambuf_type* __obuf = nullptr)
+  _LIBCPP_HIDE_FROM_ABI basic_syncbuf()
+      : basic_syncbuf(nullptr) {}
+
+  _LIBCPP_HIDE_FROM_ABI explicit basic_syncbuf(streambuf_type* __obuf)
       : basic_syncbuf(__obuf, _Allocator()) {}
 
   _LIBCPP_HIDE_FROM_ABI basic_syncbuf(streambuf_type* __obuf, _Allocator const& __alloc)

Copy link

github-actions bot commented Jul 20, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff ce5620ba9a5bf48bce4e49933aec531c70c54aeb 0398e15da74b888316809a30d694ae3638625552 --extensions ,cpp -- libcxx/include/syncstream libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/cons.default.pass.cpp
View the diff from clang-format here.
diff --git a/libcxx/include/syncstream b/libcxx/include/syncstream
index a0617f4acf..fea4c66b8e 100644
--- a/libcxx/include/syncstream
+++ b/libcxx/include/syncstream
@@ -255,11 +255,9 @@ public:
 
   // [syncstream.syncbuf.cons], construction and destruction
 
-  _LIBCPP_HIDE_FROM_ABI basic_syncbuf()
-      : basic_syncbuf(nullptr) {}
+  _LIBCPP_HIDE_FROM_ABI basic_syncbuf() : basic_syncbuf(nullptr) {}
 
-  _LIBCPP_HIDE_FROM_ABI explicit basic_syncbuf(streambuf_type* __obuf)
-      : basic_syncbuf(__obuf, _Allocator()) {}
+  _LIBCPP_HIDE_FROM_ABI explicit basic_syncbuf(streambuf_type* __obuf) : basic_syncbuf(__obuf, _Allocator()) {}
 
   _LIBCPP_HIDE_FROM_ABI basic_syncbuf(streambuf_type* __obuf, _Allocator const& __alloc)
       : __wrapped_(__obuf), __str_(__alloc) {

Comment on lines +258 to +259
_LIBCPP_HIDE_FROM_ABI basic_syncbuf()
: basic_syncbuf(nullptr) {}
Copy link
Member

Choose a reason for hiding this comment

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

I think it's possible to test this. Before applying the LWG issue resolution, the following code would not have compiled:

basic_syncbuf f() {
  return {};
}

But after the LWG issue resolution, this compiles because the default constructor is not explicit. So I think a test like this should work:

basic_syncbuf buf = {};

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the return {} test better, so went that direction for the test.

[libc++][syncbuf] Implements LWG3253.

Note the tests already tested this behaviour and the wording change is NFC.

Implements
- LWG3253 basic_syncbuf::basic_syncbuf() should not be explicit
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

I rebased onto main. LGTM, merging.

@ldionne ldionne merged commit f4ea19b into llvm:main Aug 30, 2024
10 of 13 checks passed
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.

LWG3253: basic_syncbuf::basic_syncbuf() should not be explicit
3 participants