This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

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
1063

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.

aaron.ballman added inline comments.Dec 22 2020, 6:28 AM
clang/include/clang/ASTMatchers/ASTMatchFinder.h
118

The fact that it's optional is a bit weird. Given that this is a temporary API, I see two approaches: hold our nose because the ugliness shouldn't be long-lived, or have the default be specified in two places with comments saying "don't forget to update over here if you change this value" so we can get rid of the Optional<> here. WDYT?

clang/lib/ASTMatchers/ASTMatchFinder.cpp
1062–1068

Elide the braces.

1080–1086

Elide the braces.

steveire added inline comments.Dec 22 2020, 8:25 AM
clang/include/clang/ASTMatchers/ASTMatchFinder.h
118

My preference is to keep the optional. Having multiple places to update makes it likely they'll get out of sync.

aaron.ballman added inline comments.Dec 22 2020, 10:14 AM
clang/include/clang/ASTMatchers/ASTMatchFinder.h
118

Then I think it's fine to leave it as Optional<> given that it's a temporary API for easing the transition. @sammccall, are you okay with that?

In terms of eventually removing this API, what's your plan? I noticed we can't do something nice like slapping a [[deprecated]] attribute on the declaration and have overriders get a notification that the method they're overriding is deprecated. Will we just pull the API entirely and force users to react to the code breakage?

steveire added inline comments.Dec 22 2020, 10:23 AM
clang/include/clang/ASTMatchers/ASTMatchFinder.h
118

Will we just pull the API entirely and force users to react to the code breakage?

Yes, I think something like that.

aaron.ballman accepted this revision.Dec 22 2020, 10:45 AM

LGTM! I'm assuming that @sammccall may be busy given the holiday season, so I think it's fine to land this as-is if you don't hear from him in the next day. If he has additional concerns, I believe we can address them pretty easily post-commit.

This revision is now accepted and ready to land.Dec 22 2020, 10:45 AM
This revision was landed with ongoing or failed builds.Feb 5 2021, 6:04 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Feb 5 2021, 6:48 AM

This breaks check-clang everywhere: http://45.33.8.238/linux/38812/step_7.txt

Please run tests before landing.

Please take a look, and revert for now if it takes a while to fix.