[PATCH] D47467: [IR] Begin removal of TerminatorInst by removing successor manipulation.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc marked 3 inline comments as done.
chandlerc added inline comments.


================
Comment at: llvm/include/llvm/IR/CFG.h:193
+
+  inline bool operator==(const Self &x) const { return Idx == x.Idx; }
+
----------------
dberris wrote:
> chandlerc wrote:
> > dberris wrote:
> > > Maybe this should be a non-member friend, and works with const and non-const versions of compatible type?
> > > 
> > > ```
> > > friend inline bool operator==(const Self& L, const Self& R) {
> > >   return L.Idx == R.Idx;
> > > }
> > > ```
> > It has to be a member for the underlying iterator helper to work (due to how that underlying helper is implemented).
> > 
> > Is there really a problem to solve here though? Not sure what the benefit is of this in the absence of a type hierarchy... We don't do it very consistently in LLVM anywhere.
> Ah, that's fair -- didn't know that the facade requires equality to be defined as a member.
> 
> The only problems the friend declaration solves are the overload resolution issue (if you want to have type deduction of the RHS to support slightly different types) and forces ADL when defined inline -- i.e. the only way to find the operator is through ADL.
> 
> Consider:
> 
> ```
> template <class T>
> class C {
>   template <class U> friend bool operator==(const C& L, const C<U>& R) {
>     return ...;
>   }
>   ...
> };
> ```
> 
> This allows C<int> and C<const int> types to be comparable, without having to make any special considerations. It also only allows for the equality comparison to be found with ADL, and no other way (AFAICT).
> 
> Whether that needs to be more widely adopted in LLVM/Clang is a separate issue, probably not worth spending a lot more time on.
Yeah, I think this is going to be fine for our somewhat limited needs for now. Thanks for the feedback though!


================
Comment at: llvm/include/llvm/IR/Instructions.h:3263
 
+  iterator_range<succ_op_iterator> successors() {
+    return make_range(
----------------
rnk wrote:
> What are these used for? It doesn't seem like they are called in any way in this code. Do these keep existing calls to `Br->successors()` compiling?
Correct. THere were a few places that were calling it directly on a BranchInst and it seemed easy enough to implement that in a more obvious fashion than going through the (quite high cost) generic successors logic.


Repository:
  rL LLVM

https://reviews.llvm.org/D47467