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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/iosfwd:100
+using osyncstream = basic_osyncstream<char>;
+using wosyncstream = basic_osyncstream<wchar_t>;
+
----------------
Can you comment these classes and typedefs have been added in C++20?


================
Comment at: libcxx/include/syncstream:52
+        basic_syncbuf& operator=(basic_syncbuf&&);
+        void swap(basic_syncbuf&);
+
----------------
@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.



================
Comment at: libcxx/include/syncstream:226
+    _VSTD::mutex& __mtx = __get_map_mtx();
+    _VSTD::lock_guard<_VSTD::mutex> __gaurd(__mtx);
+
----------------
Minor nit `__gaurd` -> `__guard`.


================
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;
----------------
Is it intended to use `assert` in a header?


================
Comment at: libcxx/include/syncstream:238
+    _VSTD::mutex& __mtx = __get_map_mtx();
+    _VSTD::lock_guard<_VSTD::mutex> __gaurd(__mtx);
+
----------------
Minor nit `__gaurd` -> `__guard`. Please grep for this typo, there's at least one more occurrence.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67086