[libcxx-commits] [PATCH] D113998: [libcxx][NFC] Add tests for std::map and std::multimap key_comp and value_comp

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

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.


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

https://reviews.llvm.org/D113998