<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Apr 12, 2017 at 12:39 AM Chandler Carruth via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">chandlerc marked 3 inline comments as done.<br class="gmail_msg">
chandlerc added a comment.<br class="gmail_msg">
<br class="gmail_msg">
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.<br class="gmail_msg"></blockquote><div><br>Yep, all makes sense - thanks!<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/ADT/iterator.h:175<br class="gmail_msg">
+ "Subscripting is only defined for random access iterators.");<br class="gmail_msg">
+ return ReferenceProxy(static_cast<DerivedT *>(this)->operator+(n));<br class="gmail_msg">
+ }<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Is there a reason these operators are written out explicitly like this, rather than as:<br class="gmail_msg">
><br class="gmail_msg">
> *static_cast<DerivedT*>(this) + n<br class="gmail_msg">
><br class="gmail_msg">
> ? I guess it makes it clearer this is calling into a function implemented by the derived class? *shrug* no worries.<br class="gmail_msg">
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.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/IR/Instructions.h:3181-3182<br class="gmail_msg">
+<br class="gmail_msg">
+ protected:<br class="gmail_msg">
+ CaseHandleT Case;<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Why protected? This class isn't derived from anywhere that I can see.<br class="gmail_msg">
Just leftover from when it was a base class. Removed.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/IR/Instructions.h:3185<br class="gmail_msg">
+ public:<br class="gmail_msg">
+ typedef CaseIteratorT<CaseHandleT> Self;<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Is this necessary? Or could you use the injected class name for example:<br class="gmail_msg">
><br class="gmail_msg">
> static CaseIteratorT fromSuccessorIndex(...<br class="gmail_msg">
This was in the old iterator class and I just didn't remove it. Nuked.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/IR/Instructions.h:3237-3238<br class="gmail_msg">
+ }<br class="gmail_msg">
+ CaseHandleT &operator*() { return Case; }<br class="gmail_msg">
+ const CaseHandleT &operator*() const { return Case; }<br class="gmail_msg">
+ };<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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)<br class="gmail_msg">
Interesting idea. This will impact other iterators though so I'd like to save it for a subsequent change.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/IR/Instructions.h:3327-3329<br class="gmail_msg">
+ for (CaseIt I = case_begin(), E = case_end(); I != E; ++I)<br class="gmail_msg">
+ if (I->getCaseValue() == C)<br class="gmail_msg">
+ return I;<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Could potentially use range-based llvm::find_if here.<br class="gmail_msg">
><br class="gmail_msg">
> 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)<br class="gmail_msg">
><br class="gmail_msg">
> But both of these are more cleanups that are only tangential to your change.<br class="gmail_msg">
Since I'm here and making these iterators work like iterators with things like range based algos, the find_if makes total sense, done.<br class="gmail_msg">
<br class="gmail_msg">
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.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/IR/Instructions.h:3346-3352<br class="gmail_msg">
+ for (auto Case : cases())<br class="gmail_msg">
+ if (Case.getCaseSuccessor() == BB) {<br class="gmail_msg">
+ if (CI)<br class="gmail_msg">
+ return nullptr; // Multiple cases lead to BB.<br class="gmail_msg">
+ else<br class="gmail_msg">
+ CI = Case.getCaseValue();<br class="gmail_msg">
}<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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)<br class="gmail_msg">
I don't follow. When we find the duplicate we return rather than continuing to consider all cases?<br class="gmail_msg">
<br class="gmail_msg">
Anyways, happy to clean this up in a follow-up if there is something here.<br class="gmail_msg"></blockquote><div><br>Ah, no - I see now (thanks for the explanation). Might be more legible/explicit with 2*find_if, but that's totally up to you.<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D31548" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D31548</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>