[libcxx-commits] [PATCH] D67086: Implement syncstream (p0053)

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/syncstream:52
+        basic_syncbuf& operator=(basic_syncbuf&&);
+        void swap(basic_syncbuf&);
+
----------------
Mordante wrote:
> @mclow.lists Do you know whether these functions should be `noexcept`? There's a contraction in the standard
> http://eel.is/c++draft/syncstream.syncbuf#assign
> ```
>    // [syncstream.syncbuf.assign], assignment and swap
>     basic_syncbuf& operator=(basic_syncbuf&&);
>     void swap(basic_syncbuf&);
> ```
> http://eel.is/c++draft/syncstream.syncbuf#assign
> `basic_syncbuf& operator=(basic_syncbuf&& rhs) noexcept;`
> `void swap(basic_syncbuf& other) noexcept;`
> 
> The move constructor is not marked as `noexcept` and neither is `emit` so I assume [syncstream.syncbuf.assign] is wrong.
> 
See https://cplusplus.github.io/LWG/issue3498 I'm going to leave them as not-noexcept until we determine a resolution for this. 


================
Comment at: libcxx/include/syncstream:230
+    auto __mtx_ptr = __mtx_ptr_map[__ptr];
+    assert(__mtx_ptr != nullptr && "This should never be nullptr");
+    return __mtx_ptr;
----------------
Mordante wrote:
> Is it intended to use `assert` in a header?
Fixed. Changed to a `_LIBCPP_ASSERT`


================
Comment at: libcxx/include/syncstream:283
+basic_syncbuf<_CharT, _Traits, _Allocator>::operator=(basic_syncbuf&& __other)
+{
+    __wrapped = _VSTD::move(__other.get_wrapped());
----------------
Mordante wrote:
> http://eel.is/c++draft/syncstream.syncbuf#assign-1 "Effects : Calls emit()..." I don't see that call.
Good catch!


================
Comment at: libcxx/include/syncstream:283
+basic_syncbuf<_CharT, _Traits, _Allocator>::operator=(basic_syncbuf&& __other)
+{
+    __wrapped = _VSTD::move(__other.get_wrapped());
----------------
zoecarver wrote:
> Mordante wrote:
> > http://eel.is/c++draft/syncstream.syncbuf#assign-1 "Effects : Calls emit()..." I don't see that call.
> Good catch!
I wonder if the same goes for `basic_osyncstream& operator=(basic_osyncstream&&) noexcept;`. It seems like that operator just isn't specified at all... another LWG issue :P


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67086/new/

https://reviews.llvm.org/D67086