[libcxx-commits] [PATCH] D92790: [libc++][P0053R7] Add <syncstream> header and implement std::basic_syncbuf

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius requested changes to this revision.
curdeius added a comment.
This revision now requires changes to proceed.

As a general note, the test paths should match the paragraphs in the standard.
So, e.g. `libcxx/test/std/input.output/stream.buffers/syncbuf/syncbuf.assign/member_swap.pass.cpp` should be `libcxx/test/std/input.output/syncstream/syncstream.syncbuf/syncstream.syncbuf.assign/swap.pass.cpp`.
@ldionne, please correct me on that if I'm wrong.

Also, you seem to lack a test on the destructor (that it correctly calls emit and doesn't throw).

Anyway, in general, I like your implementation.



================
Comment at: libcxx/docs/Cxx2aStatusPaperStatus.csv:196
 "`P2116 <https://wg21.link/P2116>`__","LWG","Remove tuple-like protocol support from fixed-extent span","Prague","|Complete|","11.0"
+"`P0053R7 <https://wg21.link/P0053R7>`__","LWG","<syncstream>","Albuquerque","|Partial|","12.0"
----------------
It would be nice if you added a note on what's implemented and what's not (see `Cxx2aStatus.rst`).


================
Comment at: libcxx/include/syncstream:14
+/*
+    syncstream synopsis
+
----------------
You should also update the synopsis of `<iosfwd>`. Cf. paragraph 3.2 of the paper.


================
Comment at: libcxx/include/syncstream:64-98
+  template <class charT,
+            class traits,
+            class Allocator>
+  class basic_osyncstream
+    : public basic_ostream<charT, traits>
+  {
+  public:
----------------
I never know, should unimplemented parts be present in the synopsis? @ldionne?


================
Comment at: libcxx/include/syncstream:135-137
+    basic_syncbuf(streambuf_type* __obuf = nullptr,
+                  const allocator_type& __a = allocator_type{})
+        : __mtx(__obuf), __buf(string_type{__a}), __wrapped(__obuf) {}
----------------
This ctor should be splitted in two, one of them being explicit and taking 0 or 1 params, cf. synopsis.


================
Comment at: libcxx/test/std/input.output/stream.buffers/syncbuf/syncbuf.cons/default.pass.cpp:17
+
+// basic_syncbuf(streambuf_type* obuf);
+
----------------
Also here, please add tests for explicit. Cf. below.


================
Comment at: libcxx/test/std/input.output/stream.buffers/syncbuf/syncbuf.cons/wrapped.pass.cpp:17
+
+// basic_syncbuf();
+
----------------
You should also test that you can't construct it implicitly. You may use `test_convertible` for that.
You might have a look at D91292.


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

https://reviews.llvm.org/D92790