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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this!



================
Comment at: libcxx/test/std/containers/sequences/vector/compare.fail.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
ldionne wrote:
> 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.
@rarutyun I'm also interested to learn more about how you measured the coverage. The coverage available at http://lab.llvm.org:8080/coverage/coverage-reports/index.html misses a lot of files and I'm not convinced the values it shows are correct. So I would be very interested to get the correct values.


================
Comment at: libcxx/test/std/containers/sequences/vector/compare.pass.cpp:117
+        assert((std::vector<int>() >= std::vector<int>()));
+    }
+}
----------------
Please add a `return 0;`. We require this for `main` since some platforms require this and we like to keep our tests portable.


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

https://reviews.llvm.org/D111738