[libcxx-commits] [PATCH] D99141: [libcxx] adds `std::incrementable_traits` to <iterator>

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Can you fix the Apple build failures? (Guess it's caused by the parent commit.)



================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/incrementable_traits.pass.cpp:222
+static_assert(!incrementable_traits_has_difference_type<
+              rvalue_ref_cv_subtraction const volatile>);
+
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > cjdb wrote:
> > > Quuxplusone wrote:
> > > > My usual complaints about hundreds of lines of tests, which test very little.
> > > > Please remove //a large percentage// of lines 94–222, and replace them with:
> > > > 
> > > > - One test that fails when line 458's `const _Tp& __x` is replaced with `const _Tp x`. (You currently lack any.)
> > > > - One test that fails when line 458's `const _Tp& __x` is replaced with `_Tp& x`. (You currently lack any.)
> > > > - One test that fails when line 458's `const _Tp& __x` is replaced with `_Tp x`. (You currently lack any.)
> > > > My usual complaints about hundreds of lines of tests, which test very little.
> > > > Please remove a large percentage of lines 94–222
> > > 
> > > As mentioned in previous patches: I have been asked to //add// tests, and my comprehensive coverage has been //encouraged// by several others, including two maintainers.
> > > 
> > > There are many lines, yes, but each of these tests ensures that cv-qualifiers and reference qualifiers are correctly handled.
> > > 
> > > > and replace them with:
> > > > * One test that fails when line 458's const _Tp& __x is replaced with const _Tp x. (You currently lack any.)
> > > > * One test that fails when line 458's const _Tp& __x is replaced with _Tp& x. (You currently lack any.)
> > > > * One test that fails when line 458's const _Tp& __x is replaced with _Tp x. (You currently lack any.)
> > > 
> > > I have added these three and more.
> > I don't feel opposition to lots of tests, even if they might seem excessive. Though, it might be good to have a few simple tests at the top or bottom that are easily verifiable and human-readable that just show "this works right in the common case".  That being said, this patch LGTM (but let's make sure Arthur is also OK with it).
> Well, I wouldn't say I'm "OK" with it; but I agree I haven't got the clout to block this patch series until it looks the way I think it should.
> 
> The number of tests in a patch should generally be proportional to the number of lines-of-code changed: here Chris is adding 24 LOC, so the tests should be on that order of magnitude.
> In any given patch, each test should usually guard against some specific failure mode; failure modes are usually of the form "what happens if line X of the patch bit-rots under maintenance?" So for example, "what happens if line 458's `const _Tp&` is replaced with `_Tp`?" Tests that can't be traced back to a specific failure mode are automatically suspect, because in my experience (both here and elsewhere in life) "hundreds of tests" is often a substitute for "any tests that actually test something."
> 
> Another way to put it is, test-code is just like ordinary code. Writing hundreds of lines of code can be useful, but it can also indicate that you need to take a step back to look at the problem you're trying to solve. Once you understand the problem being solved, you might find that an elegant fix is available.
> 
> The hundred-line rough draft can still be useful! Looking at Chris's "rough draft" with its hundreds of lines of `const_lvalue_ref_volatile_rvalue_ref_subtraction` gave me the clue "okay, cvref-qualifiers are important to this code somehow." Then I looked at the problem being solved — "okay, we need to test that something might bit-rot in this code, //in the area of cvref-qualifiers specifically//." That's how I found that none of these tests were testing what they intended. The next step is to write a surgical patch that solves the problem; I think I can write the new test for each of the three bullet points above in about 5 lines each. With the smaller patch in hand, the big patch is no longer technically necessary — and therefore I personally wouldn't check it in. (Although, again, I understand that I'm not the gatekeeper here, and the big patch is quite likely to get checked in regardless.)
I'm personally also not too fond of the amount of tests and I agree with @Quuxplusone. However some maintainers have asked for verbose tests for some of the concepts in earlier reviews. Maybe we should discuss on discord what we expect of unit tests and how verbose. I don't like to block this review on that discussion; the tests aren't wrong and we can always reduce the number of tests later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99141