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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver accepted this revision as: zoecarver.
zoecarver added a comment.

Thanks!



================
Comment at: libcxx/include/iterator:472
+
+// TODO(cjdb): add iter_difference_t once iterator_traits is cleaned up.
+#endif // _LIBCPP_STD_VER > 17
----------------
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. 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99141