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

[Louis Dionne via Phabricator via libcxx-commits]

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

I'd like to put an end to comments about "too many tests" once and for all. I think we all agree that test coverage is a good property to have. I think we also agree that readability is a good property to have, and that code duplication is bad. We want both, as much as possible.

I think what we're seeing here is an example where we have a lot of code coverage, which is great, however the cost (right now) is a lot of repetition, and hence decreased readability. For me, the solution is not to remove the test coverage, but instead to implement it without reducing readability so much. Here's an example of how this can be done (there are other ways, for example we could use templates instead but I just hacked this together). This replaces lines 169 to 702. Yes it's still not *small*, but it's much better and IMO easier to read, while not dropping any of the coverage. And it would be easy to make it even better by using two nested "loops" over all qualifiers, but it's unclear to me that would actually increase readability.

  #define TEST_NOT_DIFFERENCE_TYPE(qual1, qual2)                                    \
    struct TEST_CONCAT(test_subtraction_,__LINE__) {                                     \
      friend int operator-(TEST_CONCAT(test_subtraction_,__LINE__) qual1,                \
                           TEST_CONCAT(test_subtraction_,__LINE__) qual2);               \
    };                                                                              \
    static_assert(!check_has_difference_type<TEST_CONCAT(test_subtraction_,__LINE__)>)   \
  /**/
  
  TEST_NOT_DIFFERENCE_TYPE(&&, &&);
  TEST_NOT_DIFFERENCE_TYPE(&, &);
  TEST_NOT_DIFFERENCE_TYPE(&, const&);
  TEST_NOT_DIFFERENCE_TYPE(&, volatile&);
  TEST_NOT_DIFFERENCE_TYPE(&, const volatile&);
  TEST_NOT_DIFFERENCE_TYPE(&, &&);
  TEST_NOT_DIFFERENCE_TYPE(&, const&&);
  TEST_NOT_DIFFERENCE_TYPE(&, volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(&, const volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(const&, &);
  TEST_NOT_DIFFERENCE_TYPE(const&, volatile&);
  TEST_NOT_DIFFERENCE_TYPE(const&, &&);
  TEST_NOT_DIFFERENCE_TYPE(const&, const&&);
  TEST_NOT_DIFFERENCE_TYPE(const&, volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(const&, const volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(volatile&, &);
  TEST_NOT_DIFFERENCE_TYPE(volatile&, const&);
  TEST_NOT_DIFFERENCE_TYPE(volatile&, volatile&);
  TEST_NOT_DIFFERENCE_TYPE(volatile&, const volatile&);
  TEST_NOT_DIFFERENCE_TYPE(volatile&, &&);
  TEST_NOT_DIFFERENCE_TYPE(volatile&, const&&);
  TEST_NOT_DIFFERENCE_TYPE(volatile&, volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(volatile&, const volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&, &);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&, volatile&);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&, &&);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&, const&&);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&, volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&, const volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(&&, &);
  TEST_NOT_DIFFERENCE_TYPE(&&, const&);
  TEST_NOT_DIFFERENCE_TYPE(&&, volatile&);
  TEST_NOT_DIFFERENCE_TYPE(&&, const volatile&);
  TEST_NOT_DIFFERENCE_TYPE(&&, &&);
  TEST_NOT_DIFFERENCE_TYPE(&&, const&&);
  TEST_NOT_DIFFERENCE_TYPE(&&, volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(&&, const volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(const&&, &);
  TEST_NOT_DIFFERENCE_TYPE(const&&, const&);
  TEST_NOT_DIFFERENCE_TYPE(const&&, volatile&);
  TEST_NOT_DIFFERENCE_TYPE(const&&, const volatile&);
  TEST_NOT_DIFFERENCE_TYPE(const&&, &&);
  TEST_NOT_DIFFERENCE_TYPE(const&&, const&&);
  TEST_NOT_DIFFERENCE_TYPE(const&&, volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(const&&, const volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(volatile&&, &);
  TEST_NOT_DIFFERENCE_TYPE(volatile&&, const&);
  TEST_NOT_DIFFERENCE_TYPE(volatile&&, volatile&);
  TEST_NOT_DIFFERENCE_TYPE(volatile&&, const volatile&);
  TEST_NOT_DIFFERENCE_TYPE(volatile&&, &&);
  TEST_NOT_DIFFERENCE_TYPE(volatile&&, const&&);
  TEST_NOT_DIFFERENCE_TYPE(volatile&&, volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(volatile&&, const volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&&, &);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&&, const&);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&&, volatile&);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&&, const volatile&);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&&, &&);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&&, const&&);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&&, volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&&, const volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(&, /*by value*/);
  TEST_NOT_DIFFERENCE_TYPE(const&, /*by value*/);
  TEST_NOT_DIFFERENCE_TYPE(volatile&, /*by value*/);
  TEST_NOT_DIFFERENCE_TYPE(&&, /*by value*/);
  TEST_NOT_DIFFERENCE_TYPE(const&&, /*by value*/);
  TEST_NOT_DIFFERENCE_TYPE(volatile&&, /*by value*/);
  TEST_NOT_DIFFERENCE_TYPE(const volatile&&, /*by value*/);
  TEST_NOT_DIFFERENCE_TYPE(/*by value*/, &);
  TEST_NOT_DIFFERENCE_TYPE(/*by value*/, const&);
  TEST_NOT_DIFFERENCE_TYPE(/*by value*/, volatile&);
  TEST_NOT_DIFFERENCE_TYPE(/*by value*/, &&);
  TEST_NOT_DIFFERENCE_TYPE(/*by value*/, const&&);
  TEST_NOT_DIFFERENCE_TYPE(/*by value*/, volatile&&);
  TEST_NOT_DIFFERENCE_TYPE(/*by value*/, const volatile&&);

WDYT? If we have to do this sort of thing frequently, we could also bundle a few utilities under `support/` to reduce duplication further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99141