[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.


================
Comment at: clang/lib/AST/Expr.cpp:1946-1947
   for (const CastExpr *E = this; E; E = dyn_cast<ImplicitCastExpr>(SubExpr)) {
     SubExpr = skipImplicitTemporary(E->getSubExpr());
+    SubExpr = SubExpr->IgnoreImplicit();
 
----------------
aaron.ballman wrote:
> `IgnoreImplicit()` calls `IgnoreImplicitSingleStep()` eventually, and that call does the same work as `skipImplicitTemporary()`, so I think the end result here should be the same.
As I look at this a second time, I just realized...calling IgnoreImplicit here mean that the loop only ever runs one iteration, since IgnoreImplicit presumably skips over ImplicitCastExprs.  While I liked how Kim did revision initially because it seemed to handle the constructor conversions similarly to their handling of getSubExprAsWritten() above, now I think something different is needed here.

Proposed alternative:
Right now skipIimplicitTemporary does what IgnoreImplicit does *except* skip over a) ImplicitCastExprs and b) FullExprs (= ConstantExprs or ExprWithCleanups).

Kim has identified that we need to skip over at least ConstantExprs at least in this case (i.e. the most conservative possible fix would be to skip over ConstantExprs just before the cast in line 1950).

But perhaps the better solution, to forestall future bugs, is to skip over FullExprs in skipImplicitTemporary, so that it skips over everything IgnoreImplicit does except ImplicitCastExprs.  (And, some documentation should be added to `skipImplicitTemporary` to that effect, to aid future maintenance.)

I can't see offhand how the other uses of skipImplicitTemporary would be negatively affected by additionally skipping over FullExprs.

Aaron what do you think?  Kim can you verify this alternative would also solve the problem without breaking any tests?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391