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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Seems some of the comments I marked as done were not registered in Phab. Reviewed the entire patch again, all should be done now.



================
Comment at: libcxx/include/map:1702-1703
 {
+    // TODO investigate this clang-tidy warning.
+    // NOLINTNEXTLINE(bugprone-use-after-move)
     return __tree_.__emplace_unique_key_args(__k,
----------------
ldionne wrote:
> Can you file a Github issue and link it here? In the issue you can say something like "we have a potential use-after-move in `map::operator[]`" and link to https://godbolt.org/z/e1x9c1h6Y.
https://github.com/llvm/llvm-project/issues/70696


================
Comment at: libcxx/include/syncstream:157
+#  ifndef _LIBCPP_HAS_NO_THREADS
+class __wrapped_streambuf_mutex {
+  _LIBCPP_HIDE_FROM_ABI __wrapped_streambuf_mutex() = default;
----------------
ldionne wrote:
> Mordante wrote:
> > ldionne wrote:
> > > This seems to be a generic utility to lock addresses. We could extract it from `<syncstream>` and make it generic, and then reuse it from here in `<syncstream>`. That way we could potentially reuse this code if we have a need for it in the future.
> > > 
> > > Or we could also maybe even abstract away the fact that we're containing a `mutex` inside the `value_type`. Then this would be a generic map from addresses to some payload.
> > I prefer to do this in a followup commit. I feel the commit is already quite large and I prefer generic tools not to be hidden in these commits.
> Can we file a Github issue and add a TODO here to look into this? That way, we have something actionable to follow up on in a PR in the near future after shipping this patch.
https://github.com/llvm/llvm-project/issues/70697


================
Comment at: libcxx/include/syncstream:441-444
+using std::syncbuf;
+#  ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
+using std::wsyncbuf;
+#  endif
----------------
ldionne wrote:
> I think these can be removed since they are in `iosfwd` already.
Actually this header should provide them. When not doing that the std modules fail, so I reverted this part and the other using.


================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.assign/assign.pass.cpp:17
+
+// basic_syncbuf& operator=(basic_syncbuf&& rhs) noexcept;
+
----------------
ldionne wrote:
> We don't seem to be testing that this is `noexcept`, and also I don't think we test the return type + return value of the function (with the usual `same_as<basic_syncbuf&> decltype(auto) ret = (assignment); assert(&ret == &obj);`).
Actually it's not a `noexcept` function per https://cplusplus.github.io/LWG/issue3867. I had already updated the header but not the synopsis here. I added the other return type test.


================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/const.move.pass.cpp:33
+      Buf b{nullptr, alloc};
+      Buf buf{std::move(b)};
+
----------------
ldionne wrote:
> ```
> Buf buf = std::move(b);
> ```
> 
> For implicit-ness?
As mentioned I prefer this way.


================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/const.move.pass.cpp:135
+  return 0;
+}
----------------
ldionne wrote:
> Not attached to this file: can we add a `dtor.pass.cpp` test that ensures that we call `emit()`?
I've added the test based on the emit test.


================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.members/emit.pass.cpp:27
+void test_set_emit_on_sync() {
+  // set_emit_on_sync tested in sync
+  test_syncbuf<T, std::allocator<T>> buff(nullptr, std::allocator<T>());
----------------
ldionne wrote:
> I can't find a test for `sync`. There is a protected method `sync()` but I don't see a test for it either, I think.
Updated the comment, it was already tested.


================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.members/helpers.h:32-36
+template <class T>
+struct test_allocator : std::allocator<T> {
+  int id;
+  test_allocator(int _id = 0) : id(_id) {}
+};
----------------
ldionne wrote:
> Can we replace this by the usual `test_allocator`?
This is again a special allocator. As mentioned during a live review we should look at the number of "test allocators" we have and try to get a good set. I feel that's out of scope for this patch.


================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/types.compile.pass.cpp:36-41
+static_assert(
+    std::same_as<std::basic_syncbuf<char>, std::basic_syncbuf<char, std::char_traits<char>, std::allocator<char>>>);
+static_assert(std::same_as<std::basic_syncbuf<char, constexpr_char_traits<char>>,
+                           std::basic_syncbuf<char, constexpr_char_traits<char>, std::allocator<char>>>);
+static_assert(std::same_as<std::basic_syncbuf<char, constexpr_char_traits<char>, test_allocator<char>>,
+                           std::basic_syncbuf<char, constexpr_char_traits<char>, test_allocator<char>>>);
----------------
ldionne wrote:
> I think we should remove the third assertion here.
Odd I'm 100% certain I removed them before, seems I did make an error with the previous update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67086