[libcxx-commits] [PATCH] D153427: [libc++] Add noexcept clauses to swap per P0408R7 (Efficient Access to basic_stringbuf's Buffer)

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

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



================
Comment at: libcxx/include/sstream:39-40
     basic_stringbuf& operator=(basic_stringbuf&& rhs);
-    void swap(basic_stringbuf& rhs);
+    void swap(basic_stringbuf& rhs);                                                   // before C++20
+    void swap(basic_stringbuf& rhs) noexcept(see below);                               // C++20
 
----------------
This is how we typically do these changes.


================
Comment at: libcxx/include/sstream:62
+void swap(basic_stringbuf<charT, traits, Allocator>& x,
+          basic_stringbuf<charT, traits, Allocator>& y);                   // noexcept(see below) since C++20
 
----------------
Please verify the layout looks nice after applying this comment.


================
Comment at: libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.assign/member_swap_noexcept.pass.cpp:32
+
+  using propagate_on_container_swap = std::true_type;
+};
----------------
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


================
Comment at: libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.assign/member_swap_noexcept.pass.cpp:36
+template <class T>
+struct some_alloc2 : some_alloc<T> {
+  using propagate_on_container_swap = std::false_type;
----------------
Please use descriptive names here. That makes reading the test a lot easier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153427