Page MenuHomePhabricator

Ignore ConstantExpr in IgnoreParens
ClosedPublic

Authored by rnk on Dec 18 2018, 1:56 PM.

Details

Summary

This moves it up from IgnoreParenImpCasts to IgnoreParens, so that more
helpers ignore it. For most clients, this ensures that these helpers
behave the same with and without C++17 enabled, which is what appears to
introduce these new expression nodes.

Fixes PR39881

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Dec 18 2018, 1:56 PM
majnemer added inline comments.
clang/lib/AST/Expr.cpp
2722–2725 ↗(On Diff #178781)

Just curious, why not go even further and use FullExpr rather than ConstantExpr?

rnk marked an inline comment as done.Dec 18 2018, 2:59 PM
rnk added inline comments.
clang/lib/AST/Expr.cpp
2722–2725 ↗(On Diff #178781)

That would include MaterializeTemporaryExpr. Do you think we should consider that a noop cast? You could make the argument that it is, but I was aiming to match pre ConstantExpr behavior.

rsmith added inline comments.Dec 18 2018, 4:48 PM
clang/lib/AST/Expr.cpp
2722–2725 ↗(On Diff #178781)

MaterializeTemporaryExpr is not a FullExpr. It would include ExprWithCleanups, though, and ExprWithCleanups is almost exclusively used by CodeGen, which probably shouldn't be calling this function.

rsmith added inline comments.Dec 18 2018, 4:49 PM
clang/lib/AST/Expr.cpp
2694 ↗(On Diff #178781)

Maybe IgnoreParens should be doing this itself?

rnk marked an inline comment as done.Dec 19 2018, 1:52 PM
rnk added inline comments.
clang/lib/AST/Expr.cpp
2694 ↗(On Diff #178781)

I can do that, it passes tests.

rnk updated this revision to Diff 178960.Dec 19 2018, 1:53 PM
  • move to IgnoreParens
rnk retitled this revision from Ignore ConstantExpr in IgnoreParenNoopCasts to Ignore ConstantExpr in IgnoreParens.Dec 19 2018, 1:53 PM
rnk edited the summary of this revision. (Show Details)
rnk added a comment.Dec 21 2018, 4:09 PM

Want to stamp this? It's 4pm on the Friday before Christmas, what could go wrong? :)

rsmith accepted this revision.Dec 21 2018, 4:28 PM

LGTM with s/ConstantExpr/FullExpr/.

clang/lib/AST/Expr.cpp
2550 ↗(On Diff #178960)

Does this pass the tests if you use FullExpr here instead? I don't think we should be treating ConstantExpr and other FullExprs differently.

This revision is now accepted and ready to land.Dec 21 2018, 4:28 PM
rnk marked 4 inline comments as done.Dec 26 2018, 9:47 AM
rnk added inline comments.
clang/lib/AST/Expr.cpp
2550 ↗(On Diff #178960)

It does not. This is the list of test failures:


Failing Tests (19):

Clang :: Analysis/cfg-rich-constructors.cpp
Clang :: Analysis/cfg-rich-constructors.mm
Clang :: Analysis/copy-elision.cpp
Clang :: Analysis/dtor.cpp
Clang :: Analysis/inlining/temp-dtors-path-notes.cpp
Clang :: Analysis/inner-pointer.cpp
Clang :: Analysis/malloc-free-after-return.cpp
Clang :: Analysis/missing-bind-temporary.cpp
Clang :: Analysis/temp-obj-dtors-cfg-output.cpp
Clang :: Analysis/temp-obj-dtors-option.cpp
Clang :: Analysis/temporaries.cpp
Clang :: Analysis/use-after-move.cpp
Clang :: CodeGenCXX/condition.cpp
Clang :: CodeGenCXX/temp-order.cpp
Clang :: CodeGenObjC/arc.m
Clang :: SemaCXX/attr-noreturn.cpp
Clang :: SemaCXX/return-noreturn.cpp
Clang :: SemaCXX/uninit-variables-conditional.cpp
Clang :: SemaCXX/warn-consumed-analysis.cpp

I think this shows that there are callers of IgnoreParens looking for an ExprWithCleanups.

This revision was automatically updated to reflect the committed changes.