[libcxx-commits] [PATCH] D111738: [libcxx][test] Add tests for std::vector comparisons

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a subscriber: cjdb.
Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/containers/sequences/vector/compare.fail.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
@rarutyun wrote:
> We have some amount of patches that increases code coverage of LLVM libc++ and we would like to contribute those

My kneejerk reaction to this PR, initially, was "I don't think this adds value," since it's mostly testing the ill-formed cases' diagnostics. But you're saying that this PR increases code coverage according to some measurable metric? Could you elaborate a bit on how you're measuring code coverage (especially for heavily templated code like this)?


================
Comment at: libcxx/test/std/containers/sequences/vector/compare.pass.cpp:16
+// bool operator> (const vector& lhs, const vector& rhs);
+// bool operator>=(const vector& lhs, const vector& rhs);
+
----------------
I'd like someone to confirm that this test is still expected to pass in C++20 after `vector`'s `operator<=>` lands. (@cjdb was working on that a long time ago in D80904.) It would be mildly unfortunate if we had to revisit this test in like a month from now.


================
Comment at: libcxx/test/support/test_comparisons.h:201
+  }
+};
+
----------------
In isolation, I'd say "please inline these types into the one test file that uses them." But I'm guessing that you're planning to submit similar followup PRs for `deque`, `list`, etc. etc., which will also use these types?
Still, these can be simplified:
- I don't see the benefit of the `int Dummy` parameter in the first two types.
- I don't see the benefit of `int second` in `LessAndEqComp`.
- I don't think the cost/benefit ratio is right for the `.fail.cpp` test; I'd eliminate it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111738