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

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

Quuxplusone added inline comments.


================
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>);
+
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99141