Page MenuHomePhabricator

Fix clang-tidy patterns to adapt to newly added ExprWithCleanups nodes.
ClosedPublic

Authored by timshen on Jun 10 2016, 1:55 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

timshen updated this revision to Diff 60401.Jun 10 2016, 1:55 PM
timshen retitled this revision from to Fix clang-tidy patterns to adapt to newly added ExprWithCleanups nodes..
timshen updated this object.
timshen added a reviewer: rsmith.
timshen added a subscriber: cfe-commits.
timshen edited reviewers, added: alexfh, bkramer, sbenza, angelgarcia; removed: rsmith.Jun 10 2016, 2:43 PM
timshen added a subscriber: rsmith.
alexfh edited edge metadata.Jun 13 2016, 5:09 AM

Does this just fix the tests broken with D20498 or have you looked at other checks that might be broken by D20498, but don't have relevant tests? In the latter case we'd certainly need your help figuring out where further problems may appear, e.g. what specific patterns should we look for in the AST matchers used in clang-tidy checks or other Clang tools. Does this boil down to expr(X) -> expr(ignoringExprWithCleanups(X))?

Does this just fix the tests broken with D20498 or have you looked at other checks that might be broken by D20498, but don't have relevant tests? In the latter case we'd certainly need your help figuring out where further problems may appear, e.g. what specific patterns should we look for in the AST matchers used in clang-tidy checks or other Clang tools. Does this boil down to expr(X) -> expr(ignoringExprWithCleanups(X))?

Actually, the more interesting question is what patterns in the analyzed code generate ExprWithCleanups?

Does this just fix the tests broken with D20498 or have you looked at other checks that might be broken by D20498, but don't have relevant tests?

This just fixes the tests broken by D20498. I didn't look at other patterns.

Actually, the more interesting question is what patterns in the analyzed code generate ExprWithCleanups?

A MaterializeTemporaryExpr now will always generate an ExprWithCleanups at full-expr entry. That is to say, there must be an ExprWithCleanups as an ancestor node of a MaterializeTemporaryExpr.

For example, the previous pattern:

fooStmt(has(
  barExpr(
    hasDescendant(
      materializeTemporaryExpr()))))

should be changed to:

fooStmt(has(
  exprWithCleanups(
    barExpr(
      hasDescendant(
        materializeTemporaryExpr())))

There are patterns around that are not explicitly checking for a MaterializeTemporaryExpr. If in the checked AST there is a MaterializeTemporaryExpr, an ignoringExprWithCleanups should be added to each of these patterns.

Does this help?

timshen updated this revision to Diff 60992.Jun 16 2016, 10:12 AM
timshen edited edge metadata.

Updated the patch to use ignorngImplicit/Expr::IgnoreImplicit matcher.

I think there is a systematic way to do this change:
Look at every ignoringImpCasts, ignoringParenCasts, ignoringParenImpCasts, ignoringParens, and
Expr::IgnoreImpCasts, Expr::IgnoreParenCasts, Expr::IgnoreParenImpCasts, Expr::IgnoreParens to
see if they can be changed to ignoringImplicit or Expr::IgnoreImplicit (which also ignores
ExprWithCleanups).

I'd say for most of the cases ExprWithCleanups should be ignored, but I'm not sure if I'm the
right person to look at these patterns.

alexfh accepted this revision.Jun 17 2016, 5:50 AM
alexfh edited edge metadata.

Yes, you're probably not the right person to fix our test coverage. Thanks for fixing the stuff that broke and I hope we'll figure out what else needs to be fixed in a finite time after the change is committed ;)

LG

This revision is now accepted and ready to land.Jun 17 2016, 5:50 AM
This revision was automatically updated to reflect the committed changes.