[PATCH] D31548: [IR] Redesign the case iterator in SwitchInst to actually be an iterator and to expose a handle to represent the actual case rather than having the iterator return a reference to itself.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks great to me, FWIW. Love getting rid of these weird pseudo iterators (or iterators with extra features in this case).



================
Comment at: include/llvm/ADT/iterator.h:175
+                  "Subscripting is only defined for random access iterators.");
+    return ReferenceProxy(static_cast<DerivedT *>(this)->operator+(n));
+  }
----------------
Is there a reason these operators are written out explicitly like this, rather than as:

  *static_cast<DerivedT*>(this) + n

? I guess it makes it clearer this is calling into a function implemented by the derived class? *shrug* no worries.


================
Comment at: include/llvm/IR/Instructions.h:3181-3182
+
+  protected:
+    CaseHandleT Case;
+
----------------
Why protected? This class isn't derived from anywhere that I can see.


================
Comment at: include/llvm/IR/Instructions.h:3185
+  public:
+    typedef CaseIteratorT<CaseHandleT> Self;
+
----------------
Is this necessary? Or could you use the injected class name for example:

  static CaseIteratorT fromSuccessorIndex(...


================
Comment at: include/llvm/IR/Instructions.h:3237-3238
+    }
+    CaseHandleT &operator*() { return Case; }
+    const CaseHandleT &operator*() const { return Case; }
+  };
----------------
Could the adapter help avoid the need for writing both of these? (by implementing the non-const version with the const version and a const_cast of the result)


================
Comment at: include/llvm/IR/Instructions.h:3327-3329
+    for (CaseIt I = case_begin(), E = case_end(); I != E; ++I)
+      if (I->getCaseValue() == C)
+        return I;
----------------
Could potentially use range-based llvm::find_if here.

If you have a way to make a CaseIt from a ConstCaseIt (even if it's private/implementation detail) - you could implement the non-const one in terms of the const one to remove duplicate code) - looks pretty plausible given the underlying representation's the same (a pointer to the SwitchInst and the index into its cases)

But both of these are more cleanups that are only tangential to your change.


================
Comment at: include/llvm/IR/Instructions.h:3346-3352
+    for (auto Case : cases())
+      if (Case.getCaseSuccessor() == BB) {
+        if (CI)
+          return nullptr; // Multiple cases lead to BB.
+        else
+          CI = Case.getCaseValue();
       }
----------------
This loop visits all elements when if there were a duplicate it could stop after it found the first one. Two calls to find_if might be nicer... maybe not. (& again, tangential refactoring not key to this change)


================
Comment at: include/llvm/IR/Instructions.h:3347
+    for (auto Case : cases())
+      if (Case.getCaseSuccessor() == BB) {
+        if (CI)
----------------
Early continue rather than nesting, maybe.


================
Comment at: include/llvm/IR/Instructions.h:3349-3350
+        if (CI)
+          return nullptr; // Multiple cases lead to BB.
+        else
+          CI = Case.getCaseValue();
----------------
else after return


https://reviews.llvm.org/D31548