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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked an inline comment as done.
cjdb added inline comments.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/incrementable_traits.pass.cpp:31-36
+template <class T, class Expected>
+[[nodiscard]] constexpr bool check_incrementable_traits() noexcept {
+  constexpr bool result = difference_type_matches<T, Expected>;
+  static_assert(difference_type_matches<T const, Expected> == result);
+  return result;
+}
----------------
Quuxplusone wrote:
> Take the suggested edit.
The suggested edit doesn't add anything constructive and appears to be a stylistic preference, so there's nothing to change.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99141