dblaikie added inline comments.
================
Comment at: llvm/include/llvm/IR/BasicBlock.h:324-325
+ template <typename PHINodeU, typename BBIteratorU,
+ typename = std::enable_if_t<
+ std::is_convertible<PHINodeU *, PHINodeT *>::value>>
phi_iterator_impl(const phi_iterator_impl<PHINodeU, BBIteratorU> &Arg)
----------------
Quuxplusone wrote:
> dblaikie wrote:
> > BRevzin wrote:
> > > dblaikie wrote:
> > > > What tripped over/required this SFINAE?
> > > There's somewhere which compared a const iterator to a non-const
> > > iterator, that ends up doing conversions in both directions under C++20
> > > rules, one direction of which is perfectly fine and the other was a hard
> > > error. Need to make the non-const iterator not constructible from a const
> > > iterator.
> > Is this true for all iterators? Or some quirk of how this one is
> > written/used (that could be fixed/changed there instead)?
> IMO there is a (much) bigger task hiding here, which is to audit every type
> in the codebase whose name contains the string "Iterator" and compare them to
> the C++20 Ranges `std::forward_iterator` concept. My impression is that the
> vast majority of real-world "iterator types" are not iterators according to
> C++20 Ranges, and that this can have arbitrarily weird effects when you mix
> them with the C++20 STL.
>
> However, that is //massive// scope creep re this particular patch. I think
> the larger question of "do all our iterators need X / are all our iterators
> written wrong" should be scoped-outside-of this patch.
Sorry, not suggesting that kind of scope creep - but do want to understand
whether this is representative of the way code should generally be written, or
whether this is working around some other issue/different fix.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78938/new/
https://reviews.llvm.org/D78938
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
