[libcxx-commits] [PATCH] D140550: [String] Refactor unit tests to allow easy addition of new allocators

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks for the patch. It took some time to look at it, so it must have been pretty tedious to write it, but I think it's a good improvement overall.

Given the huge number of changes, I might have made the changes without touching the formatting at all to reduce the diff as much as possible, and then do the formatting changes (or not at all). But anyway, they're here now and I've reviewed it like that, so let's not change it. Just something to consider for the future.

Let's try to get this landed quickly to avoid getting stuck in endless rebase conflict resolutions.



================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:60-61
 
-template <class CharT>
-constexpr bool test() {
-  using S = std::basic_string<CharT>;
+template <class CharT, template <class> class Alloc>
+constexpr bool test_string() {
+  using S = std::basic_string<CharT, std::char_traits<CharT>, Alloc<CharT> >;
----------------
Instead, I would do:

```
template <class String>
constexpr bool test() {
  test_appending<String>(...);
  ...
}
```

WDYT?



================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:88-102
+  test_string<char, std::allocator>();
+
+  test_string<char8_t, std::allocator>();
+
+  test_string<char16_t, std::allocator>();
+
+  test_string<char32_t, std::allocator>();
----------------
The whitespace between `test_string` calls and `static_assert`s seems superfluous.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/T_size_size.pass.cpp:112
+template <>
+TEST_CONSTEXPR_CXX20 void test_string<char, test_allocator>() {
+  using A  = test_allocator<char>;
----------------
We generally don't specialize function templates. This is a red flag to me. Why do we need this specialization?


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/copy.pass.cpp:39
+template <>
+TEST_CONSTEXPR_CXX20 void test_string<char, test_allocator>() {
+  using A = test_allocator<char>;
----------------
Same comment about specializing.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/iter_alloc.pass.cpp:61
+template <class Alloc>
+TEST_CONSTEXPR_CXX20 void test(const Alloc& a) {
+  const char* s = "12345678901234567890123456789012345678901234567890";
----------------
There's a functional difference here -- we used to use a different allocator object for each call to `test(....)`, and now they all share the same `a`. Instead, you could perhaps use `test(s, s, Alloc(a))` (etc...).

This situation arises in several tests, so whatever we do to fix it, you should apply it consistently throughout this patch.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/string_view_deduction.pass.cpp:38-41
+struct CanDeduce<StringView,
+                 Allocator,
+                 decltype((void)std::basic_string{std::declval<StringView>(), std::declval<Allocator>()})>
+    : std::true_type {};
----------------
Please conserve the previous formatting, it highlighted the expression being checked much better.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/string_view_size_size_deduction.pass.cpp:42-48
+struct CanDeduce<
+    StringView,
+    Size,
+    Allocator,
+    decltype((void)std::basic_string{
+        std::declval<StringView>(), std::declval<Size>(), std::declval<Size>(), std::declval<Allocator>()})>
+    : std::true_type {};
----------------
Same: conserve formatting.


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_assign/iterator.pass.cpp:80-83
+  test(S(),
+       cpp17_input_iterator<const char*>(s),
+       cpp17_input_iterator<const char*>(s + 52),
+       S("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"));
----------------
Please keep each of these calls a single line, even if that makes it a long line. It makes the structure clearer.


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_insert/iter_iter_iter.pass.cpp:79
+  test(S("12345678901234567890"), 15, s, s + 10, S("123456789012345ABCDEFGHIJ67890"));
+  test(S("12345678901234567890"),
+       20,
----------------
Same comment about line breaks.


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/iter_iter_iter_iter.pass.cpp:1071
+}
\ No newline at end of file

----------------
I don't think this change is intended.


================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/iter_iter_pointer_size.pass.cpp:1011
+}
\ No newline at end of file

----------------
Same. Multiple files have this issue, please check them all.


================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string.accessors/get_allocator.pass.cpp:36
+template <>
+TEST_CONSTEXPR_CXX20 void test_string<char, test_allocator>() {
+  using A = test_allocator<char>;
----------------
Function template specialization = red flag.


================
Comment at: libcxx/test/support/test_allocator.h:23
 
 template <class Alloc>
 TEST_CONSTEXPR_CXX20 inline typename std::allocator_traits<Alloc>::size_type alloc_max_size(Alloc const& a) {
----------------
The changes to this file are purely formatting, so I'd drop them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140550