[Clang-tidy] Make modernize-redundant-void-arg working with included files

[Eugene.Zelenko]

Summary

This fix for PR25894. I checked it on my work code base.

Build and regressions were OK on RHEL 6.

[aaron.ballman]

Missing testcases for this; I suspect that isExpansionInMainFile was being used to filter out results in macro expansions, so some tests to ensure the behavior is correct there would also be appreciated.

[Eugene.Zelenko]

I didn't notice test cases for included files for other checkers. So it's hard to tell for should special test case introduced for this specific problem or not.

[aaron.ballman]

In D16786#341938, @Eugene.Zelenko wrote:

I didn't notice test cases for included files for other checkers. So it's hard to tell for should special test case introduced for this specific problem or not.

Generally speaking, all changes should have a test case accompanying them unless the change is NFC. It helps ensure we don't regress behavior the we want with the fix.

[Eugene.Zelenko]

I think proper solution will be to create tests for included files ot the fly, bu renaming main test to .h and creating dummy source file. But this is task for scripts wizards :-)

[aaron.ballman]

In D16786#342072, @Eugene.Zelenko wrote:

I think proper solution will be to create tests for included files ot the fly, bu renaming main test to .h and creating dummy source file. But this is task for scripts wizards :-)

I'm not certain if that's the proper solution or not, but I'm also not comfortable with committing nontrivial changes without an accompanying test case. either. I would recommend writing a simple test include that would fail before applying your patch (and succeeds after) and go that route, unless you want to try your hand at the scripting approach.

[Eugene.Zelenko]

In D16786#342074, @aaron.ballman wrote:

In D16786#342072, @Eugene.Zelenko wrote:

I think proper solution will be to create tests for included files ot the fly, bu renaming main test to .h and creating dummy source file. But this is task for scripts wizards :-)

I'm not certain if that's the proper solution or not, but I'm also not comfortable with committing nontrivial changes without an accompanying test case. either. I would recommend writing a simple test include that would fail before applying your patch (and succeeds after) and go that route, unless you want to try your hand at the scripting approach.

My point is that all checks should be test in similar situations. It's not reasonable to introduce special checks only for selected ones.

[aaron.ballman]

In D16786#342075, @Eugene.Zelenko wrote:

In D16786#342074, @aaron.ballman wrote:

In D16786#342072, @Eugene.Zelenko wrote:

I think proper solution will be to create tests for included files ot the fly, bu renaming main test to .h and creating dummy source file. But this is task for scripts wizards :-)

I'm not certain if that's the proper solution or not, but I'm also not comfortable with committing nontrivial changes without an accompanying test case. either. I would recommend writing a simple test include that would fail before applying your patch (and succeeds after) and go that route, unless you want to try your hand at the scripting approach.

My point is that all checks should be test in similar situations. It's not reasonable to introduce special checks only for selected ones.

Agreed; however, not all checks have the same behavior regarding macro expansion and header files. Some checks have special behaviors regarding macros, others may require the check to not be run outside of the main file, etc. I'm not certain how well such a test harness can be generalized for this, but if you have ideas in mind, they would be great to explore!

[LegalizeAdulthood]

I am already working on fixing this bug (isn't PR25894 already assigned to me?), but what's missing from this patch is an automated test that proves it works.

In order to write such an automated test, I believe some enhancements are needed to the check_clang_tidy.py script before such a test can be written.

[Eugene.Zelenko]

Obsoleted by D16953.

clang-tidy/modernize/RedundantVoidArgCheck.cpp

 namespace modernize {
 
 void RedundantVoidArgCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(functionDecl(isExpansionInMainFile(), parameterCountIs(0),
-                                  unless(isImplicit()), unless(isExternC()))
-                         .bind(FunctionId),
+  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),
+      cxxConstCastExpr(CastDestinationIsFunction).bind(NamedCastId),
       this);
-  Finder->addMatcher(lambdaExpr(isExpansionInMainFile()).bind(LambdaId), this);
+  Finder->addMatcher(lambdaExpr().bind(LambdaId), this);
 }
 
 void RedundantVoidArgCheck::check(const MatchFinder::MatchResult &Result) {