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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks a lot for looking into this! I was incredibly surprised to learn that we didn't have tests for `std::vector`'s comparison. It's pretty vexing.

If you have similar patches for other container types, it's probably reasonable to fold them into this one (assuming they are all pretty similar).



================
Comment at: libcxx/test/std/containers/sequences/vector/compare.fail.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Quuxplusone wrote:
> @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)?
@Quuxplusone I mean, the negative comparison test we can have a discussion about, but the positive test is definitely increasing coverage. I was surprised when I was unable to find tests for `vector`'s comparison operators, and it seems like indeed we didn't have any before this patch.

@rarutyun Regarding the negative tests, what Arthur is saying is that we don't normally assert the ill-formedness of things. On reason is that there's almost always an infinite number of ways a program can be ill-formed, so where do you stop? If the Standard did specify that `operator==(vector<T>, vector<U>)` **does not participate in overload resolution** if `T` and `U` are not comparable, then we'd test that. But as it is, the fact that `T` and `U` are `Cpp17EqualityComparable` is a precondition (http://eel.is/c++draft/description#structure.specifications-3.3), which means that it's technically UB to perform the comparisons you do below.

I think my preferred approach here would be to drop the negative tests but definitely keep the positive ones.


================
Comment at: libcxx/test/std/containers/sequences/vector/compare.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Is it possible that you've made these files executable? See the `File Mode` diff above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111738