[PATCH] D78938: Make LLVM build in C++20 mode

[David Blaikie via Phabricator via cfe-commits]

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