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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/iterator:489
+    incrementable_traits<remove_cvref_t<_Ip>>,
+    iterator_traits<remove_cvref_t<_Ip>>>::difference_type;
 
----------------
tcanens wrote:
> cjdb wrote:
> > 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.
> > I don't think you should be instantiating `incrementable_traits<RI>` in the case that `iterator_traits<RI>` is the primary template. 
> 
> What does this even mean? How do you produce `incrementable_­traits<RI>::difference_­type` without instantiating `incrementable_­traits<RI>`?
I flipped the logic by accident. I should have said, don't instantiate `incrementable_traits<RI>` in the case that `iterator_traits<RI>` is **not** the primary template, i.e. in the case when we want to just go ahead and trust `iterator_traits<RI>::value_type`. (And I almost made the exact same error again in typing up this message! Note to self: Here "primary" means "the one we //don't// want to use.")


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99863