While doing this, I also found a few tests that were either clearly
incorrect (e.g. testing the wrong function) or that lacked basic test
coverage like testing std::string itself (e.g. the test was only checking
std::basic_string with a custom allocator). In these cases, I did a few
conservative drive-by changes.
Details
- Reviewers
bemeryesr ldionne - Group Reviewers
Restricted Project - Commits
- rG6e1dcc933511: [libc++] Refactor string unit tests to ease addition of new allocators
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp | ||
---|---|---|
61–62 | Instead, I would do: template <class String> constexpr bool test() { test_appending<String>(...); ... } WDYT? | |
87–96 | The whitespace between test_string calls and static_asserts seems superfluous. | |
libcxx/test/std/strings/basic.string/string.cons/T_size_size.pass.cpp | ||
112 | We generally don't specialize function templates. This is a red flag to me. Why do we need this specialization? | |
libcxx/test/std/strings/basic.string/string.cons/copy.pass.cpp | ||
39 | Same comment about specializing. | |
libcxx/test/std/strings/basic.string/string.cons/iter_alloc.pass.cpp | ||
59 | 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. | |
libcxx/test/std/strings/basic.string/string.cons/string_view_deduction.pass.cpp | ||
38–41 ↗ | (On Diff #485057) | Please conserve the previous formatting, it highlighted the expression being checked much better. |
libcxx/test/std/strings/basic.string/string.cons/string_view_size_size_deduction.pass.cpp | ||
42–48 ↗ | (On Diff #485057) | Same: conserve formatting. |
libcxx/test/std/strings/basic.string/string.modifiers/string_assign/iterator.pass.cpp | ||
57–60 | Please keep each of these calls a single line, even if that makes it a long line. It makes the structure clearer. | |
libcxx/test/std/strings/basic.string/string.modifiers/string_insert/iter_iter_iter.pass.cpp | ||
79 ↗ | (On Diff #485057) | Same comment about line breaks. |
libcxx/test/std/strings/basic.string/string.modifiers/string_replace/iter_iter_iter_iter.pass.cpp | ||
1071 ↗ | (On Diff #485057) | I don't think this change is intended. |
libcxx/test/std/strings/basic.string/string.modifiers/string_replace/iter_iter_pointer_size.pass.cpp | ||
1011 ↗ | (On Diff #485057) | Same. Multiple files have this issue, please check them all. |
libcxx/test/std/strings/basic.string/string.ops/string.accessors/get_allocator.pass.cpp | ||
36 | Function template specialization = red flag. | |
libcxx/test/support/test_allocator.h | ||
23 ↗ | (On Diff #485057) | The changes to this file are purely formatting, so I'd drop them. |
Thanks for the quick review! I will work on this today.
I completely agree with you about the formatting changes. I actually initially tried to have this PR just include the actual changes (and not the formatting changes) but when I submitted the PR using arc, it forced me to apply the clang formatting (or at least it seemed that way to me, perhaps I did something wrong). Is there a way to submit the patch without applying the clang formatting to all touched files?
As a side note, based on your comments about the formatting, perhaps you want to weigh in on this PR of mine: https://reviews.llvm.org/D140612.
- Remove formatting-only changes.
- Remove function template specializations.
- Address remaining feedback from Idionne.
libcxx/test/std/strings/basic.string/string.cons/T_size_size.pass.cpp | ||
---|---|---|
112 | I just added it since the test_allocator needs to be treated differently within the test (i.e. initialised with a value) and wanted to keep the calling code consistent. But I'll remove the function template specialization. | |
libcxx/test/std/strings/basic.string/string.modifiers/string_insert/iter_iter_iter.pass.cpp | ||
79 ↗ | (On Diff #485057) | This change was just formatting so I've removed it. |
I was fine with this patch back then, I still think it's good.
I'll ship it if the CI is green.
libcxx/test/std/strings/basic.string/string.cons/T_size_size.pass.cpp | ||
---|---|---|
72 | TODO: check this refactoring. | |
libcxx/test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp | ||
80 | TODO: check this one. | |
libcxx/test/std/strings/basic.string/string.cons/iter_alloc.pass.cpp | ||
89 | ||
libcxx/test/std/strings/basic.string/string.cons/move.pass.cpp | ||
25 | TODO: check this one and everything below. |
Instead, I would do:
WDYT?