[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 llvm-commits]

On Wed, Apr 12, 2017 at 12:39 AM Chandler Carruth via Phabricator <
reviews at reviews.llvm.org> wrote:

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

Yep, all makes sense - thanks!


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

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.

- Dave


>
>
> https://reviews.llvm.org/D31548
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170412/cef61acc/attachment.html>