-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[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
Conversation
@llvm/pr-subscribers-libcxx Author: Mark de Wever (mordante) ChangesNote the tests already tested this behaviour and the wording change is NFC. Implements
Full diff: https://github.com/llvm/llvm-project/pull/99778.diff 2 Files Affected:
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)
|
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) {
|
_LIBCPP_HIDE_FROM_ABI basic_syncbuf() | ||
: basic_syncbuf(nullptr) {} |
There was a problem hiding this comment.
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 = {};
There was a problem hiding this comment.
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.
ff22196
to
9d7872f
Compare
[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
9d7872f
to
0398e15
Compare
There was a problem hiding this 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.
Note the tests already tested this behaviour and the wording change is NFC.
Implements
Closes #100264