[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

This revision was automatically updated to reflect the committed changes. Closed by commit rL354215: [AST] Factor out the logic of the various Expr::Ignore* (authored by brunoricci, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

riccibruno added a comment. > FWIW, I was rather disappointed in a recent review to learn that > IgnoreParens() means "ignore parens... and a whole bunch of other stuff like > generic selection expressions". +1 Indeed. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

aaron.ballman added a comment. In D57267#1391101 , @riccibruno wrote: > > I don't think there was an explicit reason beyond "I didn't need to do it > > at the time". So probably just an oversight on my part. I don't know the > > code nearly as well as

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

riccibruno added a comment. > I don't think there was an explicit reason beyond "I didn't need to do it at > the time". So probably just an oversight on my part. I don't know the code > nearly as well as @rnk, so I could be wrong, but I think the existing tests > should tell you if something

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

rnk added a comment. In D57267#1390484 , @riccibruno wrote: > @void @rnk Perhaps you can comment on this: currently `Expr::IgnoreImpCasts` > skips `FullExpr`s, but `Expr::IgnoreParenImpCasts` only skips (via > `IgnoreParens`) `ConstantExpr`s. Is there

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

void added a comment. In D57267#1390484 , @riccibruno wrote: > @void @rnk Perhaps you can comment on this: currently `Expr::IgnoreImpCasts` > skips `FullExpr`s, but `Expr::IgnoreParenImpCasts` only skips (via > `IgnoreParens`) `ConstantExpr`s. Is there

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

riccibruno added subscribers: rnk, void. riccibruno added a comment. @void @rnk Perhaps you can comment on this: currently `Expr::IgnoreImpCasts` skips `FullExpr`s, but `Expr::IgnoreParenImpCasts` only skips (via `IgnoreParens`) `ConstantExpr`s. Is there any reason for this inconsistency ? I

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! You should give @rsmith a few days in case he wants to express an opinion though. Comment at: lib/AST/Expr.cpp:2643-2645 + } + + else if (auto *GSE =

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

riccibruno updated this revision to Diff 185539. riccibruno added a comment. Go back to using `dyn_cast`s. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57267/new/ https://reviews.llvm.org/D57267 Files: include/clang/AST/Expr.h lib/AST/Expr.cpp Index:

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

riccibruno marked an inline comment as done. riccibruno added inline comments. Comment at: lib/AST/Expr.cpp:2562 +return ICE->getSubExpr(); + + else if (auto *FE = dyn_cast_or_null(E)) aaron.ballman wrote: > riccibruno wrote: > > It is something that is

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

aaron.ballman added inline comments. Comment at: lib/AST/Expr.cpp:2562 +return ICE->getSubExpr(); + + else if (auto *FE = dyn_cast_or_null(E)) riccibruno wrote: > It is something that is actually possible to audit. I did look at where each > of the skipped

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

riccibruno updated this revision to Diff 184944. riccibruno marked 6 inline comments as done. riccibruno added a comment. Rebased and addressed Aaron's comments, except for the null check issue on which I am still undecided. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

riccibruno added inline comments. Comment at: lib/AST/Expr.cpp:2562 +return ICE->getSubExpr(); + + else if (auto *FE = dyn_cast_or_null(E)) It is something that is actually possible to audit. I did look at where each of the skipped node are created and it

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

aaron.ballman added inline comments. Comment at: lib/AST/Expr.cpp:2562 +static Expr *IgnoreImpCastsSingleStep(Expr *E) { + if (auto *ICE = dyn_cast_or_null(E)) +return ICE->getSubExpr(); riccibruno wrote: > aaron.ballman wrote: > > Do we really need to

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

riccibruno added inline comments. Comment at: lib/AST/Expr.cpp:2562 +static Expr *IgnoreImpCastsSingleStep(Expr *E) { + if (auto *ICE = dyn_cast_or_null(E)) +return ICE->getSubExpr(); aaron.ballman wrote: > Do we really need to accept null arguments? We

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

aaron.ballman added a reviewer: rsmith. aaron.ballman added a comment. Adding Richard for opinions on whether `IgnoreParenImpCasts` should skip full expressions, and also the changes to accepting null expressions as arguments in more places. Comment at: lib/AST/Expr.cpp:2559

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

riccibruno updated this revision to Diff 183696. riccibruno added a comment. Update the comment in `IgnoreImpCastsExtraSingleStep` and return early from `IgnoreImpCastsExtraSingleStep` and `IgnoreImplicitSingleStep` when `IgnoreImpCastsSingleStep` skipped something. Repository: rC Clang

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

riccibruno created this revision. riccibruno added a reviewer: aaron.ballman. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Now that the implementation of all of the `Expr::Ignore*` is in `Expr.cpp` we can try to remove some duplication. Do this by separating the