Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files

[LegalizeAdulthood]

Summary

Update check_clang_tidy.py to handle fixes applied to header files by adding --header-filter argument that:

• Passes -header-filter down to clang-tidy
• Copies named header to temporary directory where clang-tidy and cleans the CHECK lines from the header as is done for copied source files
• Passes -I . as an extra argument to clang-tidy in order to consume the copied, cleaned header
• Performs FileCheck checks on the header for CHECK-MESSAGES
• Performs FileCheck checks on the header for CHECK-FIXES

Fixes PR25894

[alexfh]

Hi Richard,

Thank you for working on this. The script has been begging for refactoring for a while ;) A couple of initial comments, and while I'm looking further into into this patch, could you find other possible usages (in currently existing tests) of the new functionality in check_clang_tidy.py?

Thanks!

test/clang-tidy/check_clang_tidy.py

82 ↗

(On Diff #47117)

Looks like this function is used not only for clang-tidy. Maybe this variable should be named more generically?

122 ↗

(On Diff #47117)

There's no need for a separate argument for this. The script passes all arguments after a -- to clang-tidy, so you can just append -- -header-filter=... -- -std=c++11 to the RUN: line in the test.

152 ↗

(On Diff #47117)

This function doesn't look like a particularly useful one: 5 lines of code instead of 2 lines and an unclear interface (in which order do the two bools come?). Please revert this part.

[LegalizeAdulthood]

test/clang-tidy/check_clang_tidy.py

122 ↗

(On Diff #47117)

This argument is the key to making everything work. It triggers the copying of the header, the cleaning of the header of CHECK-FIXES and CHECK-MESSAGES, the diffing of the header, etc.

I originally had it like you suggested by trying to have the test file pass all the necessary extra arguments to clang-tidy, but it just isn't enough. The script needs to do a bunch of extra work when headers are involved, so it seemed to make the most sense to pass the argument directly to the script so that it knows to do this extra work.

In other words, express the intent directly to the script instead of having the script infer intent by scraping through extra arguments.

Additionally, this preserves the behavior of eliminating duplicated arguments across all clang-tidy tests because the script will assume -std=c++11 for .cpp files.

152 ↗

(On Diff #47117)

I initially had it doing the same checks on the header file as well and then it made more sense. I dug myself into a dead-end on that series of changes, reverted and started over.

What I settled on here was the assumption that you won't have CHECK-MESSAGES or CHECK-FIXES in your header file, unless you also had them in the associated source file. If that assumption is invalid, then I should call this function again for the header and change the tests to be asserting fixes in the source or header, messages in the source or header in various places.

[LegalizeAdulthood]

test/clang-tidy/check_clang_tidy.py

152 ↗

(On Diff #47117)

IMO, "5 lines instead of 2 lines" isn't the kind of reasoning that is useful. Otherwise we would never do Compose Method on a long method since the act of breaking a long method down into a bunch of smaller methods necessarily introduces more lines of code.

This script originally was one giant function that did everything. A bunch of things need to be done twice now: one for the source file and once for the header file. Applying Compose Method Extracting results in extracting a group of smaller functions/methods.

Now the functions have a bunch of state that needs to be passed around between them and the functions might be more clearly expressed as methods on a class, but I didn't go that far with it.

[LegalizeAdulthood]

Update from review comments

[LegalizeAdulthood]

Squeak?

[alexfh]

Unfortunately, the review of the changes in the script might take some time. I think, we can submit the fix itself already and work on the rest of the patch after that.

[alexfh]

I've committed the changes to the check code. Can you submit the change to the script as a separate patch?

[alexfh]

And thank you for the fix, btw!

clang-tools-extra/trunk/clang-tidy/modernize/RedundantVoidArgCheck.cpp

 const char LambdaId[] = "lambda";
 
 } // namespace
 
 namespace tidy {
 namespace modernize {
 
 void RedundantVoidArgCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(functionDecl(isExpansionInMainFile(), parameterCountIs(0),
-                                  unless(isImplicit()), unless(isExternC()))
+  Finder->addMatcher(functionDecl(parameterCountIs(0), unless(isImplicit()),
+                                  unless(isExternC()))
                          .bind(FunctionId),
                      this);
-  Finder->addMatcher(typedefDecl(isExpansionInMainFile()).bind(TypedefId),
-                     this);
+  Finder->addMatcher(typedefDecl().bind(TypedefId), this);
   auto ParenFunctionType = parenType(innerType(functionType()));
   auto PointerToFunctionType = pointee(ParenFunctionType);
   auto FunctionOrMemberPointer =
       anyOf(hasType(pointerType(PointerToFunctionType)),
             hasType(memberPointerType(PointerToFunctionType)));
-  Finder->addMatcher(
-      fieldDecl(isExpansionInMainFile(), FunctionOrMemberPointer).bind(FieldId),
-      this);
-  Finder->addMatcher(
-      varDecl(isExpansionInMainFile(), FunctionOrMemberPointer).bind(VarId),
-      this);
+  Finder->addMatcher(fieldDecl(FunctionOrMemberPointer).bind(FieldId), this);
+  Finder->addMatcher(varDecl(FunctionOrMemberPointer).bind(VarId), this);
   auto CastDestinationIsFunction =
       hasDestinationType(pointsTo(ParenFunctionType));
   Finder->addMatcher(
-      cStyleCastExpr(isExpansionInMainFile(), CastDestinationIsFunction)
-          .bind(CStyleCastId),
-      this);
+      cStyleCastExpr(CastDestinationIsFunction).bind(CStyleCastId), this);
   Finder->addMatcher(
-      cxxStaticCastExpr(isExpansionInMainFile(), CastDestinationIsFunction)
-          .bind(NamedCastId),
-      this);
+      cxxStaticCastExpr(CastDestinationIsFunction).bind(NamedCastId), this);
   Finder->addMatcher(
-      cxxReinterpretCastExpr(isExpansionInMainFile(), CastDestinationIsFunction)
-          .bind(NamedCastId),
+      cxxReinterpretCastExpr(CastDestinationIsFunction).bind(NamedCastId),
       this);
   Finder->addMatcher(
-      cxxConstCastExpr(isExpansionInMainFile(), CastDestinationIsFunction)
-          .bind(NamedCastId),
-      this);
-  Finder->addMatcher(lambdaExpr(isExpansionInMainFile()).bind(LambdaId), this);
+      cxxConstCastExpr(CastDestinationIsFunction).bind(NamedCastId), this);
+  Finder->addMatcher(lambdaExpr().bind(LambdaId), this);
 }
 
 void RedundantVoidArgCheck::check(const MatchFinder::MatchResult &Result) {
   if (!Result.Context->getLangOpts().CPlusPlus) {
     return;
   }
 
   const BoundNodes &Nodes = Result.Nodes;