Page MenuHomePhabricator

Ignore implicit nodes in IgnoreUnlessSpelledInSource mode
ClosedPublic

Authored by steveire on Fri, Nov 6, 3:42 PM.

Details

Summary

Update the ASTNodeTraverser to dump only nodes spelled in source. There
are only a few which need to be handled, but Decl nodes for which
isImplicit() is true are handled together.

Update the RAV instances used in ASTMatchFinder to ignore the nodes too.
As with handling of template instantiations, it is necessary to allow
the RAV to process the implicit nodes because they need to be visitable
before the first traverse() matcher is encountered. An exception to
this is in the MatchChildASTVisitor, because we sometimes wish to make a
node matchable but make its children not-matchable. This is the case
for defaulted CXXMethodDecls for example.

This change accounts for handling nodes not spelled in source when using
direct matching of nodes, and when using the has() and hasDescendant()
matchers. Other matchers such as
cxxRecordDecl(hasMethod(cxxMethodDecl())) still succeed for
compiler-generated methods for example after this change. Updating the
implementations of hasMethod() and other matchers is for a follow-up
patch.

Diff Detail

Event Timeline

steveire created this revision.Fri, Nov 6, 3:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 6, 3:42 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steveire requested review of this revision.Fri, Nov 6, 3:42 PM

Thank you for this, I think it's mostly looking good!

clang/include/clang/AST/ASTNodeTraverser.h
88

Similar to the feedback on D90984, I think this will do the wrong thing for the weird traversal mode for ignoring parens and implicit casts. It should probably be checking for Traversal == TK_IgnoreUnlessSpelledInSource here.

192

Same issue here.

410

Same here. I'll stop calling these out -- can you double check all the uses of Traversal != TK_AsIs?

733

If we're in AsIs mode, don't we still want to match on some parts of the range-based for loop because they are spelled in source (like the body of the loop or the range-expression, etc)?

The test behavior looks reasonable around this, so I suspect I'm just misunderstanding this change.

742

This be const auto * or at least auto *.

clang/lib/ASTMatchers/ASTMatchFinder.cpp
152

Similar issue here about traversal mode.

1084

Spelling this as bool would not be amiss (same below, given how trivial it is to spell the type out).

1109

bool here as well, please.

1158

Here too.

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
2503

Can you add one more test for the body like cxxForRangeStmt(hasBody(compoundStmt())) which should match in both modes?

aaron.ballman accepted this revision.Tue, Nov 17, 7:19 AM

LGTM, thank you!

clang/lib/ASTMatchers/ASTMatchFinder.cpp
164

Good catch!

This revision is now accepted and ready to land.Tue, Nov 17, 7:19 AM
This revision was landed with ongoing or failed builds.Tue, Nov 17, 8:50 AM
This revision was automatically updated to reflect the committed changes.

(usual procedure is to revert while investigating test failures instead of commenting out the tests)

Hi. I suspect it might be this patch or D90984 that might be leading to the test failure we're seeing on our arm64 builders

FAIL: Clang-Unit :: AST/./ASTTests/Traverse.IgnoreUnlessSpelledInSourceImplicit (15871 of 26886)
******************** TEST 'Clang-Unit :: AST/./ASTTests/Traverse.IgnoreUnlessSpelledInSourceImplicit' FAILED ********************
Note: Google Test filter = Traverse.IgnoreUnlessSpelledInSourceImplicit
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from Traverse
[ RUN      ] Traverse.IgnoreUnlessSpelledInSourceImplicit
clang/unittests/AST/ASTTraverserTest.cpp:1114: Failure
      Expected: dumpASTString(TK_AsIs, TUDecl)
      Which is: "\nTranslationUnitDecl\n|-TypedefDecl '__int128_t'\n| ...

Would you mind taking a look? Thanks.

Builder link: https://luci-milo.appspot.com/p/fuchsia/builders/ci/clang-linux-arm64/b8863364626069720736

@stephenkelly This is breaking a large number of bots - please can you revert your patch series to get things green again?

I've removed the offending test (which in this case is the correct fix - it is not a temporary fix).

@RKSimon Is there some way I can see what is causing the failures on the other platforms? Was it because of this test? How can I monitor this? I knew how to see it in lab.llvm.org, but it seems things are missing or moved in the new buildbot interface. I otherwise have no way of knowing what problems were caused by this, and whether a new version of this patch fixes the problems?