Add C++20 header <syncstream> and implement std::basic_syncbuf as described in P0053R7
Details
Diff Detail
Event Timeline
This is my first contribution to libcxx/ I tried my best to match style etc but bear with me :)
libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist | ||
---|---|---|
558–572 | Not sure why make generate-cxx-abilist touched these, does anyone have any ideas? LLVM_USE_SANITIZER perhaps? |
Hi and thanks for the patch!
Please mark the new tests with // UNSUPPORTED: c++03, c++11, c++14, c++17.
Update and run utils/generate_feature_test_macro_components.py to add __cpp_lib_syncstream feature test macro.
Also please update status page for this paper in docs/.
Update should trigger CI.
libcxx/test/std/input.output/stream.buffers/syncbuf/syncbuf.assign/member_swap.pass.cpp | ||
---|---|---|
17 | You should mark all your tests as UNSUPPORTED: for all previous standards. |
I made this __cpp_lib_syncbuf looks like thats what the macro is supposed to be called, https://en.cppreference.com/w/cpp/feature_test#Library_features
Also please update status page for this paper in docs/.
Update should trigger CI.
Thanks for the suggestions.
I'm not seeing any coordination between different syncbufs on the same stream here.
My impression was that implementing this required a global registry of streams that had syscstreams associated with them.
libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist | ||
---|---|---|
558–572 | If these things have changed sizes, that's an ABI break. |
FYI, I've just landed D92656, so now you can use utils/generate_header_tests.py to generate (some) tests for a new header. You might need to modify the script to add condition on _LIBCPP_HAS_NO_THREADS. Anyway, what you did on tests looks good.
Also, notes are now at https://libcxx.llvm.org/docs/Contributing.html.
Just a few comments for the moment, I'll take time to review it a bit later.
libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist | ||
---|---|---|
35–43 | It all depends on your environment and link flags. Once CI gets triggered you'd be able to get updated ABI lists from artifacts on failed build steps (in Buildkite). You'll probably need to remove these unrelated changes (exception, align_val_t). | |
558–572 | Just to help reviewing, please mark addressed comments as "Done". |
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.
libcxx/docs/Cxx2aStatusPaperStatus.csv | ||
---|---|---|
196 | It would be nice if you added a note on what's implemented and what's not (see Cxx2aStatus.rst). | |
libcxx/include/syncstream | ||
15 | You should also update the synopsis of <iosfwd>. Cf. paragraph 3.2 of the paper. | |
65–99 | I never know, should unimplemented parts be present in the synopsis? @ldionne? | |
136–138 | This ctor should be splitted in two, one of them being explicit and taking 0 or 1 params, cf. synopsis. | |
libcxx/test/std/input.output/stream.buffers/syncbuf/syncbuf.cons/default.pass.cpp | ||
18 | Also here, please add tests for explicit. Cf. below. | |
libcxx/test/std/input.output/stream.buffers/syncbuf/syncbuf.cons/wrapped.pass.cpp | ||
18 | You should also test that you can't construct it implicitly. You may use test_convertible for that. |
- Add note to Cxx2aStatus.rst
- update synopsis of <iosfwd>
- Split ctor
- Update emit_mt.pass.cpp to use more threads
- Fix race condition in getting lock in mutex::mutex
- Fix set_emit_on_sync test which didn’t end with .pass.cpp so it wasn’t being tested previously (oops)
- Move dtor tests from emit test to its own test
- Renamed test files as suggested
Thanks a lot for the comments @curdeius
libcxx/include/syncstream | ||
---|---|---|
15 | Thanks, good catch | |
65–99 | I have removed them for now | |
libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist | ||
35–43 | How does CI get triggered for libcxx? You mentioned it in a previous comment, but will that be only after pushing the changes? |
libcxx/docs/Cxx2aStatusPaperStatus.csv | ||
---|---|---|
6 | You should update the status here instead of adding a new line. BTW, maybe drop R7 in the note. | |
libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist | ||
35–43 | Ahhh, I think I know what may happen. How do you upload the patches? You don't use arcanist, do you? |
libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist | ||
---|---|---|
35–43 | Yes - so far, it seems like if the patch already has the repo set, it still looks like it's set while updating the diff (and removing and readding it in the last step doesn't help), but after submitting, it's gone. So I guess one either has to explicitly remove the repo before updating a patch, or update it twice (so that one can re-set the repo the second time, to make CI pick it up again). FWIW, I don't think the repo setting is something specific to the libcxx hooks, but sounds to me more like a phabricator issue. |
Requesting changes so that it shows up nicely in the review queue.
@abrachet, could you please sync with Zoe and possibly merge your two patches?
I had a closer look at this patch. I think the other patch now is more complete. Abandoning it to clean up the review queue.
You should update the status here instead of adding a new line. BTW, maybe drop R7 in the note.