[PATCH] D82278: Fix traversal over CXXConstructExpr in Syntactic mode

[Stephen Kelly via Phabricator via cfe-commits]

steveire marked 2 inline comments as done.
steveire added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp:80
   auto Matches =
-      match(expr(hasDescendant(typeLoc().bind("t"))), *E, *Result.Context);
+      match(traverse(TK_AsIs, expr(hasDescendant(typeLoc().bind("t")))), *E,
+            *Result.Context);
----------------
aaron.ballman wrote:
> 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.
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.


================
Comment at: clang/lib/AST/Expr.cpp:3001
         Expr *A = C->getArg(0);
-        if (A->getSourceRange() == SR || !isa<CXXTemporaryObjectExpr>(C))
+        if (A->getSourceRange() == SR || C->isElidable()) {
           E = A;
----------------
ymandel wrote:
> aaron.ballman wrote:
> > Looks like the change introduced new curly braces for a single-line if.
> 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.
The `ignoringElidableConstructorCall` does it this way. I'm afraid I don't know more than that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82278