[libcxx-commits] [PATCH] D67086: Implement syncstream (p0053)
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Oct 3 10:41:06 PDT 2023
ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
Thanks a lot for picking up this patch! I have a bunch of comments but this is starting to look pretty good.
================
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,
----------------
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.
================
Comment at: libcxx/include/syncstream:157
+# ifndef _LIBCPP_HAS_NO_THREADS
+class __wrapped_streambuf_mutex {
+ _LIBCPP_HIDE_FROM_ABI __wrapped_streambuf_mutex() = default;
----------------
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.
================
Comment at: libcxx/include/syncstream:190
+ // pre: __ptr is in __lut_
+ [[nodiscard]] _LIBCPP_HIDE_FROM_ABI lock_guard<mutex> __get_lock([[maybe_unused]] void* __ptr) noexcept {
+ shared_lock __lock{__mutex_};
----------------
`operator[]` or `.find()` maybe? Note that `operator[]` has an unfortunate precedent of default-constructing the key if it's not present in `std::map` so we might want to be careful about re-using `operator[]`, but this is worth thinking about.
================
Comment at: libcxx/include/syncstream:198
+ // It is allowed to call this function with a non-registered pointer.
+ [[nodiscard]] _LIBCPP_HIDE_FROM_ABI size_t __get_count([[maybe_unused]] void* __ptr) noexcept {
+ _LIBCPP_ASSERT_INTERNAL(__ptr != nullptr, "non-wrapped streambufs are never written to");
----------------
If this class represents a by-address lock, perhaps this should be `bool __is_locked(void* __ptr)`.
================
Comment at: libcxx/include/syncstream:206
+
+ [[nodiscard]] static _LIBCPP_HIDE_FROM_ABI __wrapped_streambuf_mutex& __instance() noexcept {
+ static __wrapped_streambuf_mutex __result;
----------------
I think this usage of a function-local static is quite interesting. It's really important to have exactly one instance of this object in the program, otherwise the locking won't work as intended. However, I think it is possible to end up with multiple copies of that object right now:
1. If we have two TUs that use different versions of libc++, I think we could end up with e.g. `__wrapped_streambuf_mutex::__instance[v1800]::__result` and `__wrapped_streambuf_mutex::__instance[v1900]::__result`.
2. On GCC where we use `always_inline`, I think this might also end up generating multiple function-local statics but I am not certain about that. The function-local static itself might be `linkonce_odr` and deduplicated across TUs correctly.
I wonder whether we should instead be stuffing that into the `dylib` -- that would definitely remove any ambiguity about there being multiple copies of the object. And if we do that, we could also hide the implementation behind a pimpl-like idiom, that way
- we do not have to depend on `std::map` in `syncstream`
- we can change to another implementation (e.g. `std::flat_map`) in the future without impacting our ABI
Concretely, since this is pretty heavyweight anyway, I don't think that we gain much by having these methods defined in headers in the hopes that Clang would inline stuff better.
================
Comment at: libcxx/include/syncstream:238
+//
+// Therefore the allocator used in the constuctor is passed to the
+// basic_string. The class does not keep a copy of this allocator.
----------------
================
Comment at: libcxx/include/syncstream:441-444
+using std::syncbuf;
+# ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
+using std::wsyncbuf;
+# endif
----------------
I think these can be removed since they are in `iosfwd` already.
================
Comment at: libcxx/include/syncstream:524-527
+using std::osyncstream;
+# ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
+using std::wosyncstream;
+# endif
----------------
Same, can be removed!
================
Comment at: libcxx/test/std/input.output/syncstream/osyncstream/members/get_wrapped.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Are we missing a test for `rdbuf`?
================
Comment at: libcxx/test/std/input.output/syncstream/osyncstream/syncstream.osyncstream.cons/cons.move.pass.cpp:17
+
+// basic_syncbuf(basic_osyncstream&& other) noexecpt;
+
----------------
================
Comment at: libcxx/test/std/input.output/syncstream/osyncstream/syncstream.osyncstream.cons/cons.move.pass.cpp:36
+template <class CharT>
+void test() {
+ {
----------------
Let's add a test for noexcept-ness since the current WP clearly specifies it, but we might remove it in the future.
================
Comment at: libcxx/test/std/input.output/syncstream/osyncstream/syncstream.osyncstream.cons/cons.move.pass.cpp:54
+ {
+ OS os{OS{&w, alloc}};
+#if defined(_LIBCPP_VERSION) && !defined(TEST_HAS_NO_THREADS)
----------------
This tests for implicit-ness too. IMO using `std::move` here instead of a temporary makes the test clearer, but this is equivalent so your choice.
================
Comment at: libcxx/test/std/input.output/syncstream/osyncstream/syncstream.osyncstream.cons/cons.ostream.allocator.pass.cpp:34
+
+ std::allocator<CharT> alloc;
+
----------------
`const`?
================
Comment at: libcxx/test/std/input.output/syncstream/osyncstream/syncstream.osyncstream.cons/cons.ostream.allocator.pass.cpp:42
+ {
+ OS os{w, alloc};
+#if defined(_LIBCPP_VERSION) && !defined(TEST_HAS_NO_THREADS)
----------------
For explicit.
================
Comment at: libcxx/test/std/input.output/syncstream/osyncstream/syncstream.osyncstream.cons/cons.ostream.pass.cpp:34
+
+ static_assert(!std::convertible_to<OS, std::basic_ostream<CharT>&>);
+ static_assert(std::constructible_from<OS, std::basic_ostream<CharT>&>);
----------------
Reversed arguments.
================
Comment at: libcxx/test/std/input.output/syncstream/osyncstream/thread/basic.pass.cpp:18-19
+
+// void emit();
+// streambuf_type* get_wrapped() const noexcept;
+
----------------
Can you replace this by a short comment explaining what this tests?
================
Comment at: libcxx/test/std/input.output/syncstream/osyncstream/types.compile.pass.cpp:38-39
+ std::basic_osyncstream<char, constexpr_char_traits<char>, std::allocator<char>>>);
+static_assert(std::same_as<std::basic_osyncstream<char, constexpr_char_traits<char>, test_allocator<char>>,
+ std::basic_osyncstream<char, constexpr_char_traits<char>, test_allocator<char>>>);
+
----------------
Remove?
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/special.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
I think this is now tested in `cons.pointer.allocator.pass.cpp` and `cons.move.pass.cpp`?
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/sputc.pass.cpp:148
+ test<wchar_t>();
+#endif
+}
----------------
`return 0`!
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/sputn.pass.cpp:131
+ test<wchar_t>();
+#endif
+}
----------------
`return 0`
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.assign/assign.pass.cpp:17
+
+// basic_syncbuf& operator=(basic_syncbuf&& rhs) noexcept;
+
----------------
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);`).
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.assign/assign.pass.cpp:71-72
+//
+// This is used the valiate some standard requirements and libc++
+// inplementation details.
+template <class CharT, class Traits, class Allocator>
----------------
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.assign/assign.pass.cpp:105
+
+ assert(buff1.get_wrapped() == nullptr);
+
----------------
I think this is a copy-paste, you probably don't need to check this again here. Probably same thing below?
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.assign/assign.pass.cpp:107
+
+ assert(buff2.get_wrapped() == &base);
+ assert(buff2.get_allocator().id == 42);
----------------
Duplicated too. Below as well?
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.assign/assign.pass.cpp:231
+template <class CharT>
+static void test_long_write_after_assign() {
+ std::basic_stringbuf<CharT> sstr1;
----------------
Generally speaking, I find it useful to add a short plain english comment explaining these test cases, since what's being tested may not be easy to figure out a few years down the line. I also wouldn't mind if these were defined inline inside `test()` w/ the comments.
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.assign/assign.pass.cpp:258
+static void test_emit_on_assign() {
+ { // false false
+
----------------
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.assign/assign.pass.cpp:287
+
+ { // false true
+
----------------
etc..
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/cons.pointer.allocator.pass.cpp:31
+ using Buf = std::basic_syncbuf<CharT>;
+ std::allocator<CharT> alloc;
+ {
----------------
We should make this `std::allocator<CharT> const alloc;` (below too), that way the test would fail if the constructor were declared as `basic_syncbuf(streambuf_type* obuf, Allocator&);`, which would pass right now.
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/cons.pointer.allocator.pass.cpp:33
+ {
+ Buf buf{nullptr, alloc};
+ assert(buf.get_wrapped() == nullptr);
----------------
Here and below, we should use `Buf buf = {nullptr, alloc}` to test that this is an implicit constructor.
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/cons.pointer.allocator.pass.cpp:39-41
+#if defined(_LIBCPP_VERSION) && !defined(TEST_HAS_NO_THREADS)
+ assert(std::__wrapped_streambuf_mutex::__instance().__get_count(&w) == 0);
+#endif
----------------
We could change those to
```
#ifndef TEST_HAS_NO_THREADS
LIBCPP_ASSERT(...);
#endif
```
Non-blocking, just a suggestion if you like that better.
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/cons.pointer.allocator.pass.cpp:43
+ {
+ Buf buf{&w, alloc};
+#if defined(_LIBCPP_VERSION) && !defined(TEST_HAS_NO_THREADS)
----------------
So here IIUC, the following would be equally valid:
```
std::basic_stringbuf<CharT> w;
Buf buf = {&w, alloc};
```
IMO not reusing `basic_syncbuf` here would make the test clearer.
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/cons.pointer.pass.cpp:32
+
+ static_assert(!std::convertible_to<Buf, std::basic_streambuf<CharT>*>);
+ static_assert(std::constructible_from<Buf, std::basic_streambuf<CharT>*>);
----------------
I think what you want here is
```
static_assert(!std::convertible_to<std::basic_streambuf<CharT>*, Buf>);
```
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/cons.pointer.pass.cpp:41
+ {
+ Buf w;
+#if defined(_LIBCPP_VERSION) && !defined(TEST_HAS_NO_THREADS)
----------------
My comment above about reusing `Buf` applies here and everywhere else.
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/const.move.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Name of the file: `cons.move.pass.cpp` instead of `const.move.pass.cpp`?
================
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)};
+
----------------
```
Buf buf = std::move(b);
```
For implicit-ness?
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/const.move.pass.cpp:44
+ {
+ Buf b{&w, alloc};
+#if defined(_LIBCPP_VERSION) && !defined(TEST_HAS_NO_THREADS)
----------------
Consider using `b1` and `b2` or something like that to make it clearer what is what. Just a suggestion.
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/const.move.pass.cpp:135
+ return 0;
+}
----------------
Not attached to this file: can we add a `dtor.pass.cpp` test that ensures that we call `emit()`?
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.members/emit.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
You swapped the filename with the one for `set_emit_on_sync.pass.cpp`.
================
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>());
----------------
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.
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.members/emit.pass.cpp:29
+ test_syncbuf<T, std::allocator<T>> buff(nullptr, std::allocator<T>());
+ ASSERT_NOEXCEPT(buff.set_emit_on_sync(false));
+}
----------------
We should at least run the code `buff.set_emit_on_sync`, right now we don't even codegen it.
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.members/get_wrapped.pass.cpp:29-30
+ std::allocator<T> alloc;
+ const test_syncbuf<T> buff(&base, alloc);
+ assert(static_cast<test_buf<T>*>(buff.get_wrapped())->id == 42);
+ ASSERT_NOEXCEPT(buff.get_wrapped());
----------------
That way we test the return type properly.
================
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) {}
+};
----------------
Can we replace this by the usual `test_allocator`?
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.members/set_emit_on_sync.pass.cpp:27
+void test() {
+ // We do this because we want to be able to any CharT
+ CharT arr[3] = {'a', 'b', 'c'};
----------------
================
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>>>);
----------------
I think we should remove the third assertion here.
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/types.compile.pass.cpp:63-64
+ std::basic_syncbuf<wchar_t, constexpr_char_traits<wchar_t>, std::allocator<wchar_t>>>);
+static_assert(std::same_as<std::basic_syncbuf<wchar_t, constexpr_char_traits<wchar_t>, test_allocator<wchar_t>>,
+ std::basic_syncbuf<wchar_t, constexpr_char_traits<wchar_t>, test_allocator<wchar_t>>>);
+
----------------
Same here, remove.
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/virtuals.pass.cpp:14-18
+// template <class charT, class traits, class Allocator>
+// class basic_syncbuf;
+
+// basic_syncbuf(streambuf_type* obuf, const Allocator& allocator);
+// basic_syncbuf(basic_syncbuf&& other);
----------------
This synopsis is wrong. I think this is the `sync` test we were looking for earlier.
================
Comment at: libcxx/test/std/input.output/syncstream/syncbuf/virtuals.pass.cpp:36
+ out << 'a';
+ out.flush();
+
----------------
Should we comment that this calls `buff.sync()` under the hood?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67086/new/
https://reviews.llvm.org/D67086
More information about the libcxx-commits
mailing list