This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add noexcept clauses to swap per P0408R7 (Efficient Access to basic_stringbuf's Buffer)
ClosedPublic

Authored by pfusik on Jun 21 2023, 7:38 AM.

Diff Detail

Event Timeline

pfusik created this revision.Jun 21 2023, 7:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2023, 7:38 AM
pfusik requested review of this revision.Jun 21 2023, 7:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2023, 7:38 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks this smaller patch is a lot easier to quickly review!
A few minor issues, otherwise looks fine!

libcxx/include/sstream
39–40

This is how we typically do these changes.

61

Please verify the layout looks nice after applying this comment.

libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.assign/member_swap_noexcept.pass.cpp
33

I like to see a few additional tests

  • An allocator without propagate_on_container_swap
  • An allocator without is_always_equal with different values for is_empty

http://eel.is/c++draft/allocator.traits.types#9
http://eel.is/c++draft/allocator.traits.types#10
http://eel.is/c++draft/meta.unary.prop#lib:is_empty,class

37

Please use descriptive names here. That makes reading the test a lot easier.

pfusik updated this revision to Diff 533325.Jun 21 2023, 10:28 AM
pfusik marked 4 inline comments as done.

Reword comments.
More tests, descriptive classes.

In general happy one comment.

libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.assign/member_swap_noexcept.pass.cpp
93

Can you add some test where noexcept(buf.swap(buf1)) == false.

pfusik marked an inline comment as done.Jun 24 2023, 6:24 AM
pfusik added inline comments.
libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.assign/member_swap_noexcept.pass.cpp
93

See five lines up.

Mordante accepted this revision.Jun 24 2023, 8:57 AM

LGTM!

libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.assign/member_swap_noexcept.pass.cpp
93

Ah sorry I overlooked that.

This revision is now accepted and ready to land.Jun 24 2023, 8:57 AM
pfusik marked an inline comment as done.Jun 24 2023, 9:00 AM

Please land this change as I have no commit access. Thank you!

Please land this change as I have no commit access. Thank you!

I've landed it on your behalf, thanks for the patch.