Skip to content

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)

@github-actions
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
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