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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

In D99141#2669722 <https://reviews.llvm.org/D99141#2669722>, @ldionne wrote:

> 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 174 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.
>
>   ...
>
> WDYT? If we have to do this sort of thing frequently, we could also bundle a few utilities under `support/` to reduce duplication further.

Now that I know we're not afraid of macros, I'll employ this where large tests are necessary.



================
Comment at: libcxx/include/__config:851
 
+#if _LIBCPP_STD_VER < 20 || defined(_LIBCPP_HAS_NO_CONCEPTS)
+#define _LIBCPP_HAS_NO_RANGES
----------------
Quuxplusone wrote:
> Please use `<= 17`, for consistency with similar lines in this file. (We tend to guard C++2a stuff on `> 17`. I don't //think// there's anything special about the Ranges stuff in this respect, right?)
> Please use `<= 17`, for consistency with similar lines in this file.

Done.

> (We tend to guard C++2a stuff on `> 17`. I don't _think_ there's anything special about the Ranges stuff in this respect, right?)

Not sure I'm following this line of thought, so I'm leaving the comment as unresolved for now.


================
Comment at: libcxx/include/iterator:472
+
+// TODO(cjdb): add iter_difference_t once iterator_traits is cleaned up.
+#endif // _LIBCPP_STD_VER > 17
----------------
zoecarver wrote:
> Hmm. I really feel like we should have a better way to track what is implemented, what is blocked on other patches, etc. TODOs might be OK for now, but I'd like to sort this out. 
In general: yes please.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/incrementable_traits.pass.cpp:34-35
+[[nodiscard]] constexpr bool check_incrementable_traits() noexcept {
+  constexpr bool result = check_difference_type_matches<T, Expected>;
+  static_assert(check_difference_type_matches<T const, Expected> == result);
+  return result;
----------------
Quuxplusone wrote:
> Note that `T const` is the same type as `T` whenever `T` is a reference type. Therefore, any place in this file that instantiates `check_incrementable_traits<Foo&>` is suspicious — it's simply static-asserting that the difference type of `T` is the difference type of `T`.
> I suggest that you add `static_assert(!std::is_reference_v<T>)` inside this template, and then re-run the tests, and investigate/fix any failures that arise from that.
> I also suggest that if you actually expect `result` to be `true` in all cases, you should add
> 
>     assert(result);
> 
> inside this function. (Or is it okay if `result` is false, sometimes? I didn't closely examine //all// the call sites.)
> Note that `T const` is the same type as `T` whenever `T` is a reference type. Therefore, any place in this file that instantiates `check_incrementable_traits<Foo&>` is suspicious — it's simply static-asserting that the difference type of `T` is the difference type of `T`.
> I suggest that you add `static_assert(!std::is_reference_v<T>)` inside this template, and then re-run the tests, and investigate/fix any failures that arise from that.

Lines 63-70 //should// already account for this, unless you're asking we check `T&` against `T const&` and friends.

> I also suggest that if you actually expect `result` to be `true` in all cases, you should add
> 
>     assert(result);
> 
> inside this function. (Or is it okay if `result` is false, sometimes? I didn't closely examine //all// the call sites.)

Lines 89 and 90-1 expect `false`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99141