[libcxx-commits] [PATCH] D99863: [libcxx] adds `iter_difference_t` and `iter_value_t`

[Christopher Di Bella via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/iterator:489
+    incrementable_traits<remove_cvref_t<_Ip>>,
+    iterator_traits<remove_cvref_t<_Ip>>>::difference_type;
 
----------------
Quuxplusone wrote:
> Please write `RI`, not `R[I]`. (I know you're trying to reflect that the `I` in the standard is a subscript letter. However, it's not an array index, and `RI` functions semantically as a single identifier — there is no separate entity `R` to be discussed, so we don't need a name for it.)
> 
> I don't think you should be instantiating `incrementable_traits<RI>` in the case that `iterator_traits<RI>` is the primary template. Please (1) check what the Standard's intent here is, (2) make the fix if needed, and (3) add a test case that detects the fix.
> 
> Separately and presumably a moot point in this case: note that `conditional_t` is compile-time-wasting by design. Anywhere in libc++ that we need its effects and aren't mandated to use `conditional_t`, we should be using Eric's `_If<is-blah, incr-traits, iter-traits>` because it instantiates fewer templates.
> I don't think you should be instantiating `incrementable_traits<RI>` in the case that `iterator_traits<RI>` is the primary template. Please (1) check what the Standard's intent here is, (2) make the fix if needed, and (3) add a test case that detects the fix.

It's pretty much left up to us. MSVC's implementation is identical to the `conditional_t` one, for example, and @CaseyCarter understands the intention better than us all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99863