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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
