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)
----------------
dblaikie wrote:
> Quuxplusone wrote:
> > BRevzin 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)?
> > > So I undid this change to copy the exact issue that I ran into. But it
> > > actually ended up still compiling anyway. Part of the issue might be that
> > > I keep futzing with the cmake configuration since it takes me more than
> > > an hour to compile, so maybe there's some target that needed this change
> > > that I no longer compile.
> > >
> > > But the kind of problem I think this had was:
> > >
> > > ```
> > > template <typename T>
> > > struct iterator {
> > > T* p;
> > >
> > > template <typename U>
> > > iterator(iterator<U> rhs)
> > > : p(rhs.p)
> > > { }
> > >
> > > bool operator==(iterator const& rhs);
> > > };
> > >
> > > bool check(iterator<int const> a, iterator<int> b) {
> > > return a == b;
> > > }
> > > ```
> > >
> > > which compiles fine in C++17 but is ambiguous in C++20 because
> > > `b.operator==(a)` is also a candidate (even though it's not _really_ a
> > > candidate, and would be a hard error if selected). the sfinae removes the
> > > bad candidate from the set.
> > >
> > > It's true for all iterators in general in that you want `const_iterator`
> > > constructible from `iterator` but not the reverse (unless they're the
> > > same type).
> > 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.
Fair enough - don't mind keeping it in then.
================
Comment at: llvm/include/llvm/Support/BinaryStreamRef.h:124
- bool operator==(const RefType &Other) const {
- if (BorrowedImpl != Other.BorrowedImpl)
+ friend bool operator==(const RefType &Self, const RefType &Other) {
+ if (Self.BorrowedImpl != Other.BorrowedImpl)
----------------
dblaikie wrote:
> Quuxplusone wrote:
> > Is there a neat rule of thumb for when you were able to preserve the member
> > `bool Me::operator==(const Me& rhs) const` versus when you were forced to
> > change it to a hidden friend? It seems like maybe you changed it to a
> > hidden friend only in cases where `Me` was a base class, is that right?
> Be curious of the answer here - and, honestly, I'd be fine with changing them
> all to friends. It makes them more consistent - equal rank for implicit
> conversions on LHS and RHS, etc. (generally considered best practice
> basically to not define op overloads as members if they can be defined as
> non-members)
Ping on this (& I'd usually call the parameters LHS and RHS rather than Self
and Other)
================
Comment at: llvm/unittests/ADT/STLExtrasTest.cpp:465
// Check fancy pointer overload for unique_ptr
+ // Parenthesizing to_address to avoid ADL finding std::to_address
std::unique_ptr<int> V2 = std::make_unique<int>(0);
----------------
dblaikie wrote:
> jhenderson wrote:
> > Nit: trailing full stop.
> Probably more suitable to use qualify the name rather than use parens (teh
> comment's still helpful to explain why either strategy is used) - that's
> what's done with llvm::make_unique, for instance.
Ping on this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78938/new/
https://reviews.llvm.org/D78938
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
