Page MenuHomePhabricator

WIP: Add an API to simplify setting TraversalKind in clang-tidy matchers
Needs ReviewPublic

Authored by steveire on May 27 2020, 5:24 AM.

Details

Reviewers
sammccall

Diff Detail

Unit TestsFailed

TimeTest
130 mslinux > Clang-Unit.ASTMatchers/_/ASTMatchersTests::ASTMatchersTest.Finder_DynamicOnlyAcceptsSomeMatchers
Note: Google Test filter = ASTMatchersTest.Finder_DynamicOnlyAcceptsSomeMatchers [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
390 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp
60 mswindows > Clang-Unit.ASTMatchers/_/ASTMatchersTests_exe::ASTMatchersTest.Finder_DynamicOnlyAcceptsSomeMatchers
Note: Google Test filter = ASTMatchersTest.Finder_DynamicOnlyAcceptsSomeMatchers [==========] Running 1 test from 1 test case.

Event Timeline

steveire created this revision.May 27 2020, 5:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2020, 5:24 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks for putting this together!
As mentioned on the cfe-dev thread, for our out-of-tree checks we're planning to wrap with traverse() instead, to capture the full matcher logic more locally at the cost of some readability.
So if this won't be used for in-tree checks, we'd probably want to hold off on this unless someone in particular plans to use it.

clang/include/clang/ASTMatchers/ASTMatchFinder.h
118

I don't really get why this would be optional.
A check's implementation isn't really going to be robust to "whatever the default is", it's going to be tested against one or the other.
So None isn't really a sensible return value - can't the base class simply return the actual default?

clang/lib/ASTMatchers/ASTMatchFinder.cpp
1147

Writing to stderr from a library isn't ideal. (e.g. in multithreaded programs - clangd uses a wrapped stderr as its log stream, this bypasses the mutex)

steveire marked an inline comment as done.Oct 29 2020, 10:00 AM

Reviving this so it can be used to port clang-tidy checks to IgnoreUnlessSpelledInSource.

clang/include/clang/ASTMatchers/ASTMatchFinder.h
118

ASTMatchFinder doesn't know the ASTContext, so it can't access the default. That's why I made it an optional.