[libcxx-commits] [PATCH] D113998: [libcxx][NFC] Add tests for associative containers key_comp and value_comp

[Konstantin Boyarinov via Phabricator via libcxx-commits]

kboyarinov added a comment.

In D113998#3140323 <https://reviews.llvm.org/D113998#3140323>, @ldionne wrote:

> In D113998#3136978 <https://reviews.llvm.org/D113998#3136978>, @kboyarinov wrote:
>
>> In D113998#3134853 <https://reviews.llvm.org/D113998#3134853>, @ldionne wrote:
>>
>>> I did find some tests in `libcxx/test/std/containers/associative/multiset/multiset.cons/compare.pass.cpp` (and similarly for other container types). Could we incorporate the existing tests into yours and delete the old ones? IMO your new tests are more complete, the only thing I would do is use `test_less` like in the original ones.
>>
>> I am not sure that we can just remove the test `compare.pass.cpp`, because it is intended to test `std::map(const key_compare&)` constructor and uses `std::map::key_comp` to test the constructor effect. New test is for `std::map::key_comp` effects. I agree that these tests are connected, but I do not think they are interchangeable. What do you think?
>
> Oh my, you are right, I looked too quickly and missed that we were testing the constructors. In those constructor tests, can you please remove the part of the comment that looks like:
>
>   // key_compare    key_comp() const;
>   // value_compare value_comp() const;
>
> This confused me into thinking we were testing those methods there, instead of just the constructor (which is explained by the comment immediately above):
>
>   // explicit multiset(const value_compare& comp);
>   // value_compare and key_compare are the same type for set/multiset
>
> LGTM with my suggested comment cleanup to the existing constructor tests.

I have cleaned-up comments in `compare.pass.cpp` but I have figured out that this test was the only place where `std::[multi]set::key_comp()` and `std::[multi]set::value_comp()` observers were tested.
I have added the similar tests as for maps to have separate test for observers.


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

https://reviews.llvm.org/D113998