[PATCH] D78098: [CGExprAgg] Fix infinite loop in `findPeephole`

[Ehud Katz via Phabricator via cfe-commits]

ekatz marked an inline comment as done.
ekatz added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprAgg.cpp:688
+
+    op = castE;
   }
----------------
rjmccall wrote:
> ekatz wrote:
> > rjmccall wrote:
> > > ekatz wrote:
> > > > rjmccall wrote:
> > > > > I liked the structure of the old code better, in case we want to look through other kinds of expressions.  Please just add `op = castE->getSubExpr()` before the `continue`.
> > > > I see your point. I'll change that.
> > > > Though I must say that the old structure is a little strange with the `return nullptr` in the end of the loop...
> > > Oh, you know, there's also an `IgnoreParenNoopCasts` that we could just use instead of this loop if we're willing to ignore other possible expressions we might want to look through.
> > I am not familiar enough with clang's code to make the decision to remove this function.
> > Personally I wanted to fix the bug, and I guess this function is here for a reason..?
> I don't think there's anything here really worth keeping.  Please keep the function around but replace its body with something like:
> 
> ```
> op = op->IgnoreParenNoopCasts();
> if (auto castE = dyn_cast<CastExpr>(op)) {
>   if (castE->getCastKind() == kind)
>     return castE->getSubExpr();
> }
> return nullptr;
> ```
Sorry, I, kinda, misunderstood you suggestion before.
This is a really good implementation, but as you said, this function should, and I quote from its description, "look through various unimportant expressions". So I guess it is better to leave it this way (with the loop), as it is already in the base code. What do you say?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78098/new/

https://reviews.llvm.org/D78098