This is an archive of the discontinued LLVM Phabricator instance.

Fix traversal over CXXConstructExpr in Syntactic mode
ClosedPublic

Authored by steveire on Jun 21 2020, 7:18 AM.

Details

Summary

Skip over elidable nodes, and ensure that intermediate
CXXFunctionalCastExpr nodes are also skipped if they are semantic.

Diff Detail

Event Timeline

steveire created this revision.Jun 21 2020, 7:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2020, 7:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.
clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp
80 ↗(On Diff #272309)

While I believe the change is necessary here, it's not obvious to me what "hints" tell me this behavior is correct for the given matchers. None of the matchers look like they're going to care about implicit nodes, so how am I to know that AsIs is correct or not as a reviewer? As it stands, I sort of feel like I have to take it on faith that this change is correct and it looks a little suspicious because the code using the matcher is creating a fix-it at what now may be the location of an implicit node.

clang/lib/AST/Expr.cpp
2829–2830

Looks like the change introduced new curly braces for a single-line if.

clang/lib/AST/ParentMapContext.cpp
157

const auto *

Thanks for this fix!

clang/lib/AST/Expr.cpp
2829–2830

Why is it necessary to check isElidable? I think the logic here is subtle (since the AST doesn't explicitly tag implicit expressions), so please add an explanatory comment.

clang/lib/AST/ParentMapContext.cpp
163

Same here. Please comment on the logic.

steveire marked 2 inline comments as done.Oct 25 2020, 12:06 PM
steveire added inline comments.
clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp
80 ↗(On Diff #272309)

I don't know if I was wrong about it being required before, or if it was required before, but the change to this file is not required now.

clang/lib/AST/Expr.cpp
2829–2830

The ignoringElidableConstructorCall does it this way. I'm afraid I don't know more than that.

ymandel added inline comments.Oct 29 2020, 8:27 AM
clang/lib/AST/Expr.cpp
2822

nit: just return SE?

clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
1236 ↗(On Diff #300550)

is this necessary/desirable given that we reverted the default behavior? if so, should we update other tests (at least, others involving this matcher) as well?

steveire marked 2 inline comments as done.Oct 29 2020, 8:40 AM
ymandel accepted this revision.Oct 29 2020, 9:35 AM
This revision is now accepted and ready to land.Oct 29 2020, 9:35 AM
This revision was landed with ongoing or failed builds.Oct 30 2020, 5:15 AM
This revision was automatically updated to reflect the committed changes.