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

[Barry Revzin via Phabricator via cfe-commits]

BRevzin 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:
> 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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78938