This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Refactor string unit tests to ease addition of new allocators
ClosedPublic

Authored by ldionne on Dec 22 2022, 6:30 AM.

Details

Summary

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.

Diff Detail

Event Timeline

bemeryesr created this revision.Dec 22 2022, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2022, 6:30 AM
Herald added a subscriber: arphaman. · View Herald Transcript
bemeryesr updated this revision to Diff 484862.Dec 22 2022, 9:20 AM

Move wide char alias into ifdef

Apply clang formatting to changed files

bemeryesr published this revision for review.Jan 9 2023, 1:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 9 2023, 1:41 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Jan 9 2023, 10:34 AM
ldionne added a subscriber: ldionne.

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.

This revision now requires changes to proceed.Jan 9 2023, 10:34 AM

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.

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.

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.

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.

You can us the --nolint flag when updating the PR to ignore clang-format.

bemeryesr updated this revision to Diff 487802.Jan 10 2023, 7:39 AM
bemeryesr marked 13 inline comments as done.
  • Remove formatting-only changes.
  • Remove function template specializations.
  • Address remaining feedback from Idionne.
bemeryesr added inline comments.Jan 10 2023, 7:41 AM
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.

Remove whitespaces at the end of lines.

bemeryesr updated this revision to Diff 508989.Mar 28 2023, 6:43 AM

Rebasing on main

ldionne commandeered this revision.Sep 1 2023, 10:18 AM
ldionne edited reviewers, added: bemeryesr; removed: ldionne.

Commandeering to rebase.

ldionne accepted this revision.Sep 1 2023, 10:21 AM

I was fine with this patch back then, I still think it's good.

I'll ship it if the CI is green.

This revision is now accepted and ready to land.Sep 1 2023, 10:21 AM
ldionne updated this revision to Diff 555440.Sep 1 2023, 10:47 AM

Rebase onto clang-format.

ldionne added inline comments.Sep 1 2023, 11:07 AM
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.

ldionne updated this revision to Diff 557365.Sep 26 2023, 9:37 AM
ldionne retitled this revision from [String] Refactor unit tests to allow easy addition of new allocators to [libc++] Refactor string unit tests to ease addition of new allocators.
ldionne edited the summary of this revision. (Show Details)

Go over all the tests and fix some refactorings that weren't the way they should.

ldionne updated this revision to Diff 557379.Sep 26 2023, 4:10 PM

Update ignore_format.txt

This revision was landed with ongoing or failed builds.Sep 27 2023, 6:02 AM
This revision was automatically updated to reflect the committed changes.