This is an archive of the discontinued LLVM Phabricator instance.

[libc++][P0053R7] Add <syncstream> header and implement std::basic_syncbuf
AbandonedPublic

Authored by Mordante on Dec 7 2020, 2:52 PM.

Details

Reviewers
ldionne
EricWF
STL_MSFT
mclow.lists
curdeius
abrachet
Group Reviewers
Restricted Project
Restricted Project
Summary

Add C++20 header <syncstream> and implement std::basic_syncbuf as described in P0053R7

Diff Detail

Event Timeline

abrachet created this revision.Dec 7 2020, 2:52 PM
abrachet requested review of this revision.Dec 7 2020, 2:52 PM

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
571–572

Not sure why make generate-cxx-abilist touched these, does anyone have any ideas? LLVM_USE_SANITIZER perhaps?

curdeius added reviewers: Restricted Project, Restricted Project.Dec 8 2020, 2:04 AM
curdeius requested changes to this revision.Dec 8 2020, 2:15 AM
curdeius added a subscriber: curdeius.

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
16 ↗(On Diff #310026)

You should mark all your tests as UNSUPPORTED: for all previous standards.

This revision now requires changes to proceed.Dec 8 2020, 2:15 AM
abrachet updated this revision to Diff 310631.Dec 9 2020, 1:22 PM

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.

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.

curdeius set the repository for this revision to rG LLVM Github Monorepo.Dec 9 2020, 3:13 PM

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
571–572

If these things have changed sizes, that's an ABI break.

abrachet updated this revision to Diff 310725.Dec 9 2020, 5:18 PM

Add _LIBCPP_VERSION test

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.

syncbuf has an internal __mutex type which maps streambuf_type *'s to mutexes

libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist
35–43

Still not sure why these are here though.

571–572

Looks like it was asan which did this.

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).

571–572

Just to help reviewing, please mark addressed comments as "Done".

curdeius requested changes to this revision.Dec 10 2020, 3:31 AM

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
17 ↗(On Diff #310725)

Also here, please add tests for explicit. Cf. below.

libcxx/test/std/input.output/stream.buffers/syncbuf/syncbuf.cons/wrapped.pass.cpp
17 ↗(On Diff #310725)

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.

This revision now requires changes to proceed.Dec 10 2020, 3:31 AM
curdeius set the repository for this revision to rG LLVM Github Monorepo.Dec 10 2020, 1:37 PM
abrachet updated this revision to Diff 311115.Dec 10 2020, 11:04 PM
  • 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
abrachet marked 6 inline comments as done.Dec 10 2020, 11:08 PM

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?

curdeius added inline comments.
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?
It seems that the problem when uploading patches through the web GUI is that it removes the set repository (rG LLVM Github Monorepo).
@mstorsjo had the same problems, apparently updating the patch twice (and setting the repo in the middle) triggers the CI if I understood correctly.
@ldionne, can we do something to fix that?
@abrachet, can you try using arcanist (arc)? See https://www.llvm.org/docs/Phabricator.html?highlight=arcanist#id3

mstorsjo added inline comments.Dec 11 2020, 1:18 AM
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.

Hey @abrachet. Thanks for the patch. Just FYI, I've got D67086 which also implements syncbuf. Maybe we can coordinate on how to merge these patches.

curdeius requested changes to this revision.Dec 17 2020, 5:45 AM

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?

This revision now requires changes to proceed.Dec 17 2020, 5:45 AM

@abrachet are you still interested in this patch. I just restarted work on D67086. Do you want to work together or do you prefer me to finish this project?

Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2023, 11:15 AM
Mordante commandeered this revision.Sep 10 2023, 9:03 AM
Mordante added a reviewer: abrachet.

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.

Mordante abandoned this revision.Sep 10 2023, 9:03 AM
Mordante marked 2 inline comments as done.