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

Sat Oct 16 06:39:25 PDT 2021

Mordante added inline comments.

================
Comment at: libcxx/test/std/containers/sequences/vector/compare.fail.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
rarutyun wrote:
> rarutyun wrote:
> > Mordante wrote:
> > > 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.
> > > Could you elaborate a bit on how you're measuring code coverage (especially for heavily templated code like this)?
> > 
> > @Quuxplusone Sure. We used BullsEye tool. For the template code it tracks that template was instantiated at least once (with any type) and called. If, of course, you have partial template specialization for the class and this specialization has its own functions all those won't be covered unless you instantiate the class to match on that partial specialization and call the method you want to cover.
> > Then, this tool as usual makes sure that all branches within the function are covered. Nothing special IIRC.
> > 
> > >The coverage available at http://lab.llvm.org:8080/coverage/coverage-reports/index.html 
> > 
> > @Mordante What I see in the report above (if I looked correctly) it does not cover `include/` folder, only `src/` one. Thus, I believe many things might be missed. We kind of build the tests and then just say that we need `include/` folder to be covered. This is very high level view of how we do it. I don't have particular technical details on my hand right now. 
> > 
> > >So I would be very interested to get the correct values.
> > 
> > We believe in those numbers we got but this tool has drawback, though (as well as all tools we tried). It cannot instrument `constexpr` functions because it injects some special code within the body of the function that does not fulfill `constexpr` requirements. As the result BullsEye just ignores such functions. 
> > 
> > We have got an idea to remove `constexpr` with some script on the fly just to collect code coverage metrics but as you may guess it doesn't work so easy because `constexpr` functions could be called in a context where constant expression is required (e.g. template instantiation of non-type template parameter) . With such approach applied the code just fails to compile in some circumstances.
> > 
> > I think my preferred approach here would be to drop the negative tests but definitely keep the positive ones.
> 
> Ok I see you point. Basically you are saying that negative tests are good for //Constaints// but do not bring much value for preconditions //Preconditions//. That makes perfect sense to me. Now I better understand the criteria.
> 
> For further patches we would keep it in mind but if something that tests //Preconditions// remains please let us know.
> >The coverage available at http://lab.llvm.org:8080/coverage/coverage-reports/index.html 
> 
> @Mordante What I see in the report above (if I looked correctly) it does not cover `include/` folder, only `src/` one. 

Indeed, but even the `src/` looks odd.

> >So I would be very interested to get the correct values.
> 
> We believe in those numbers we got but this tool has drawback, though (as well as all tools we tried). It cannot instrument `constexpr` functions because it injects some special code within the body of the function that does not fulfill `constexpr` requirements. As the result BullsEye just ignores such functions. 

Thanks for the information!

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

https://reviews.llvm.org/D111738