[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.

[Chandler Carruth via Phabricator via llvm-commits]

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

Thanks for the review. Mostly done, landing with those updates. Couple of things left, but you indicated they could be follow-ups. Questions on exactly what to do with them below.



================
Comment at: include/llvm/ADT/iterator.h:175
+                  "Subscripting is only defined for random access iterators.");
+    return ReferenceProxy(static_cast<DerivedT *>(this)->operator+(n));
+  }
----------------
dblaikie wrote:
> 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.
What you said. I didn't want to play overload resolution games or anything. Otherwise you sometimes get a pretty awful error message listing all the candidates that didn't help and not explaining that DerivedT needs to provide something.


================
Comment at: include/llvm/IR/Instructions.h:3181-3182
+
+  protected:
+    CaseHandleT Case;
+
----------------
dblaikie wrote:
> Why protected? This class isn't derived from anywhere that I can see.
Just leftover from when it was a base class. Removed.


================
Comment at: include/llvm/IR/Instructions.h:3185
+  public:
+    typedef CaseIteratorT<CaseHandleT> Self;
+
----------------
dblaikie wrote:
> Is this necessary? Or could you use the injected class name for example:
> 
>   static CaseIteratorT fromSuccessorIndex(...
This was in the old iterator class and I just didn't remove it. Nuked.


================
Comment at: include/llvm/IR/Instructions.h:3237-3238
+    }
+    CaseHandleT &operator*() { return Case; }
+    const CaseHandleT &operator*() const { return Case; }
+  };
----------------
dblaikie wrote:
> 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)
Interesting idea. This will impact other iterators though so I'd like to save it for a subsequent change.


================
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;
----------------
dblaikie wrote:
> 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.
Since I'm here and making these iterators work like iterators with things like range based algos, the find_if makes total sense, done.

I'd like to save using the const to implement both for a subsequent change. I'm not 100% comfortable with the const casting stuff there, but I think it is probably worthwhile.


================
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();
       }
----------------
dblaikie wrote:
> 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)
I don't follow. When we find the duplicate we return rather than continuing to consider all cases?

Anyways, happy to clean this up in a follow-up if there is something here.


https://reviews.llvm.org/D31548