This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][test][NFC] Add tests for backward iteration over associative containers
ClosedPublic

Authored by kboyarinov on Jan 25 2022, 2:09 AM.

Details

Summary

Add test cases for iteration over the ordered associative container from end to begin using operator--

Diff Detail

Event Timeline

kboyarinov requested review of this revision.Jan 25 2022, 2:09 AM
kboyarinov created this revision.
kboyarinov added inline comments.Jan 25 2022, 2:20 AM
libcxx/test/std/containers/associative/map/map.ops/count.pass.cpp
110–145 ↗(On Diff #402800)

This test case is intended to improve code coverage of the libc++ implementation of map. The implementation uses helper class __map_value_compare for EBO:

template <class _Key, class _CP, class _Compare,
                  bool = is_empty_v<_Compare> && !is_final_v<_Compare>>
class __map_value_compare : private _Compare { /*...*/};

template <class _Key, class _CP, class _Compare>
class __map_value_compare<_Key, _CP, _Compare, false>
{
  _Compare comp;
  /*...*/
};

To test both generic template and the specialization, we need additional test-case for final comparator.
Do you think such testing should be organized in this manner or something should be added or changed?

libcxx/test/std/containers/associative/map/map.ops/count.pass.cpp
110–145 ↗(On Diff #402800)

Having not looked closely at all, I suggest we pull the whole body of main out into void test() { ~~~ } so that main becomes a two-liner; and then we make test into a template.

template<class Compare>
void test() {
    ~~~
}

int main(int, char**)
{
    test<std::less<int>>();
#if TEST_STD_VER >= 11
    test<FinalCompare<int>>();
#endif
#if TEST_STD_VER > 11
    test<std::less<>>();
#endif
}

Or do all the different tests here use different key types so this idea would end up being complicated?

Might also make sense to move this discussion/diff into its own separate PR, on the assumption that every other diff here is uncontroversial. (Every other diff is just testing backward iteration, right?)

kboyarinov retitled this revision from [libcxx][test][NFC] Additional test cases for associative containers to [libcxx][test][NFC] Add tests for backward iteration over associative containers.
kboyarinov edited the summary of this revision. (Show Details)

Remove test-case for the comparator marked final (moved to D118232)

LGTM if CI is green. Some nitpicks.

libcxx/test/std/containers/associative/map/map.access/iterator.pass.cpp
88

Perhaps, after line 88, assert(i == m.begin()); . Ditto throughout.

libcxx/test/std/containers/associative/multiset/iterator.pass.cpp
125
220

For consistency with line 213.

This revision is now accepted and ready to land.Jan 26 2022, 2:09 PM

Add assert(i == end()) and rename variables for consistency

kboyarinov marked 3 inline comments as done.Jan 31 2022, 7:13 AM
rarutyun accepted this revision.Feb 7 2022, 2:00 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptFeb 7 2022, 2:22 PM