[libcxx-commits] [PATCH] D94807: [libc++] Rationalize our treatment of contiguous iterators and __unwrap_iter().
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Jan 27 15:03:28 PST 2021
ldionne added inline comments.
================
Comment at: libcxx/include/iterator:609
+struct __is_cpp17_contiguous_iterator : _And<
+ __has_iterator_category_convertible_to<_Tp, random_access_iterator_tag>,
+ _Or<
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Isn't this check made redundant by the following check for `__has_iterator_category_convertible_to<_Tp, contiguous_iterator_tag>`?
> It's not completely redundant — it rejects user-defined iterator types with `iterator_category=input_iterator_tag` and `iterator_concept=contiguous_iterator_tag`. However, I can't point to any such types in the wild, and I didn't write any unit tests for such types, so I agree — I'll remove the redundancy. Let someone put it back if they run into such a case in the wild.
That would be a weird and misleading iterator, wouldn't it?
================
Comment at: libcxx/include/iterator:618
+template <class _Up>
+struct __is_cpp17_contiguous_iterator<_Up*> : __has_iterator_category_convertible_to<_Up*, random_access_iterator_tag> {};
----------------
Quuxplusone wrote:
> ldionne wrote:
> > I take it that you're not using just `true_type` here in case someone specializes `iterator_traits<MyFoo*>` to have a non random-access `iterator_category`?
> As the comment indicated, my intent had been to make sure this specialization didn't accidentally accept types like `int(*)()`, `void*`,... which are syntactically `_Up*` but not actually iterators.
> However, writing tests for this revealed that if you try to ask `__is_cpp17_contiguous_iterator<void*>` it just blows up anyway, because instantiating `iterator_traits<void*>` is ill-formed.
> So as above, I agree — I'll just do `true_type` here and let someone "fix" the corner case for real, if they ever run into it.
>
> I hadn't realized that `__has_iterator_category_convertible_to` wasn't SFINAE-friendly. (It barfs on most non-iterator types.)
> I hadn't realized that __has_iterator_category_convertible_to wasn't SFINAE-friendly. (It barfs on most non-iterator types.)
This is surprising to me. Perhaps we should fix it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94807/new/
https://reviews.llvm.org/D94807
More information about the libcxx-commits
mailing list