[clang-tidy] Improve modernize-redundant-void-arg to recognize macro uses

[LegalizeAdulthood]

Summary

Sometimes a macro invocation will look like an argument list
declaration. Improve the check to detect this situation and not
try to modify the macro invocation.

Thanks to Nathan James for the fix.

• Ignore implicit typedefs (e.g. compiler builtins)
• Improve lexing state machine to locate void argument tokens
• Add additional return_t() macro tests
• clang-format control in the test case file

Fixes #43791

[aaron.ballman]

Generally LGTM, just a testing request.

clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp

562

Can you also add a test for:

void func(return_t(void));

[LegalizeAdulthood]

clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp

562

:-)

What are you suggesting the result should be? Honestly, looking at that, I'm not sure myself :)

IMO, if I saw this in a code review, I would flag it because you're using a macro called "return type" to specify the type of an argument.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp

562

LoL, yeah, the name return_t would certainly be novel to use in a parameter list, but what I was hoping to test is whether we try to fix the use of the macro within the parameter list or not. I *think* it probably makes sense to issue the diagnostic, but I don't think it makes sense to try to fix it because the macro could be defined differently for different configurations. But the diagnostic is silenced as well as the fix-it, I wouldn't lose a whole lot of sleep over it.

[LegalizeAdulthood]

clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp

562

Well it could conceivably be used to declare a function pointer argument like this:

void func(return_t(void) (*fp)(void));

In that case, my expectation is that the check would fix the void arg, but not the arg to the macro.

[LegalizeAdulthood]

clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp

562

OK, that was a good idea to add the test I described above because it failed :),
so let me improve the check some more.

[jrtc27]

clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp

562

If you want a less-contrived example that shows up all over the place in crusty old C code that supports (or, perhaps, supported and let bitrot support for) pre-ANSI C compilers:

#define __P(x) x
void foo __P((void));

(the idea being that, for pre-ANSI C compilers, you'd instead define __P(x) as () to get void foo ();)

[LegalizeAdulthood]

clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp

562

Well, in (modern) C you don't want to remove the void in (void) at all.

This check applies only to C++ code, so we should be safe here.

I'll add a test case just to cover it -- I think the correct result should be
that the check leaves this instance of (void) intact.

[LegalizeAdulthood]

Add more test cases and improve the Lexing state machine

[aaron.ballman]

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

48–49

Might as well fix the clang-format issue.

151

I think a safer interface to this is to use a const IdentifierTable & here and in isMacroIdentifier(). Then, in isMacroIdentifier(), instead of using get() (which has no const overload because it can add an identifier to the table), you can use find(). This removes any possibility of accidentally adding identifiers to the table (even in future refactorings).

157–184

158–162

165–169

172–176

195–213

218–220

clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp

202

Unrelated change can be split out an NFC commit if you'd like, but we don't typically attempt to clang-format existing test files, so also not really necessary either.

562

Well, in (modern) C you don't want to remove the void in (void) at all.

In everything but -ansi mode, in fact (C89 supported prototypes). :-D

I'll add a test case just to cover it -- I think the correct result should be that the check leaves this instance of (void) intact.

Thanks for the coverage, and I agree, we shouldn't try mucking with a fix in the presence of a macro.

[LegalizeAdulthood]

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

158–162

Ah, found it in the style guide
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

[LegalizeAdulthood]

• clang-format
• use const IdentifierTable & to avoid accidental modification
• drop braces from simple block statements as per style guide

[aaron.ballman]

LGTM, thanks!

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

 namespace clang {
 namespace tidy {
 namespace modernize {
 
 namespace {
 
 // Determine if the given QualType is a nullary function or pointer to same.
 bool protoTypeHasNoParms(QualType QT) {
-  if (const auto *PT = QT->getAs<PointerType>()) {
+  if (const auto *PT = QT->getAs<PointerType>())
     QT = PT->getPointeeType();
-  }
-  if (auto *MPT = QT->getAs<MemberPointerType>()) {
+  if (auto *MPT = QT->getAs<MemberPointerType>())
     QT = MPT->getPointeeType();
-  }
-  if (const auto *FP = QT->getAs<FunctionProtoType>()) {
+  if (const auto *FP = QT->getAs<FunctionProtoType>())
     return FP->getNumParams() == 0;
-  }
   return false;
 }
 
 const char FunctionId[] = "function";
 const char TypedefId[] = "typedef";
 const char FieldId[] = "field";
 const char VarId[] = "var";
 const char NamedCastId[] = "named-cast";
 const char CStyleCastId[] = "c-style-cast";
 const char ExplicitCastId[] = "explicit-cast";
 const char LambdaId[] = "lambda";
 
 } // namespace
 
 void RedundantVoidArgCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(functionDecl(parameterCountIs(0), unless(isImplicit()),
                                   unless(isInstantiated()), unless(isExternC()))
                          .bind(FunctionId),
                      this);
-  Finder->addMatcher(typedefNameDecl().bind(TypedefId), this);
+  Finder->addMatcher(typedefNameDecl(unless(isImplicit())).bind(TypedefId),
+                     this);
   auto ParenFunctionType = parenType(innerType(functionType()));
   auto PointerToFunctionType = pointee(ParenFunctionType);
   auto FunctionOrMemberPointer =
       anyOf(hasType(pointerType(PointerToFunctionType)),
             hasType(memberPointerType(PointerToFunctionType)));
   Finder->addMatcher(fieldDecl(FunctionOrMemberPointer).bind(FieldId), this);
   Finder->addMatcher(varDecl(FunctionOrMemberPointer).bind(VarId), this);
   auto CastDestinationIsFunction =
       hasDestinationType(pointsTo(ParenFunctionType));
   Finder->addMatcher(
       cStyleCastExpr(CastDestinationIsFunction).bind(CStyleCastId), this);
   Finder->addMatcher(
       cxxStaticCastExpr(CastDestinationIsFunction).bind(NamedCastId), this);
   Finder->addMatcher(
       cxxReinterpretCastExpr(CastDestinationIsFunction).bind(NamedCastId),
       this);
   Finder->addMatcher(
       cxxConstCastExpr(CastDestinationIsFunction).bind(NamedCastId), this);
   Finder->addMatcher(lambdaExpr().bind(LambdaId), this);
 }
 
 void RedundantVoidArgCheck::check(const MatchFinder::MatchResult &Result) {
   const BoundNodes &Nodes = Result.Nodes;
-  if (const auto *Function = Nodes.getNodeAs<FunctionDecl>(FunctionId)) {
+  if (const auto *Function = Nodes.getNodeAs<FunctionDecl>(FunctionId))
     processFunctionDecl(Result, Function);
-  } else if (const auto *TypedefName =
-                 Nodes.getNodeAs<TypedefNameDecl>(TypedefId)) {
+  else if (const auto *TypedefName =
+               Nodes.getNodeAs<TypedefNameDecl>(TypedefId))
     processTypedefNameDecl(Result, TypedefName);
-  } else if (const auto *Member = Nodes.getNodeAs<FieldDecl>(FieldId)) {
+  else if (const auto *Member = Nodes.getNodeAs<FieldDecl>(FieldId))
     processFieldDecl(Result, Member);
-  } else if (const auto *Var = Nodes.getNodeAs<VarDecl>(VarId)) {
+  else if (const auto *Var = Nodes.getNodeAs<VarDecl>(VarId))
     processVarDecl(Result, Var);
-  } else if (const auto *NamedCast =
-                 Nodes.getNodeAs<CXXNamedCastExpr>(NamedCastId)) {
+  else if (const auto *NamedCast =
+               Nodes.getNodeAs<CXXNamedCastExpr>(NamedCastId))
     processNamedCastExpr(Result, NamedCast);
-  } else if (const auto *CStyleCast =
-                 Nodes.getNodeAs<CStyleCastExpr>(CStyleCastId)) {
+  else if (const auto *CStyleCast =
+               Nodes.getNodeAs<CStyleCastExpr>(CStyleCastId))
     processExplicitCastExpr(Result, CStyleCast);
-  } else if (const auto *ExplicitCast =
-                 Nodes.getNodeAs<ExplicitCastExpr>(ExplicitCastId)) {
+  else if (const auto *ExplicitCast =
+               Nodes.getNodeAs<ExplicitCastExpr>(ExplicitCastId))
     processExplicitCastExpr(Result, ExplicitCast);
-  } else if (const auto *Lambda = Nodes.getNodeAs<LambdaExpr>(LambdaId)) {
+  else if (const auto *Lambda = Nodes.getNodeAs<LambdaExpr>(LambdaId))
     processLambdaExpr(Result, Lambda);
-  }
-}
 
 void RedundantVoidArgCheck::processFunctionDecl(
     const MatchFinder::MatchResult &Result, const FunctionDecl *Function) {
-  if (Function->isThisDeclarationADefinition()) {
-    SourceLocation Start = Function->getBeginLoc();
-    SourceLocation End = Function->getEndLoc();
+  const auto *Method = dyn_cast<CXXMethodDecl>(Function);
+  SourceLocation Start = Method && Method->getParent()->isLambda()
+                             ? Method->getBeginLoc()
+                             : Function->getLocation();
+  if (Function->isThisDeclarationADefinition()) {
     if (const Stmt *Body = Function->getBody()) {
       End = Body->getBeginLoc();
       if (End.isMacroID() &&
           Result.SourceManager->isAtStartOfImmediateMacroExpansion(End))
         End = Result.SourceManager->getExpansionLoc(End);
       End = End.getLocWithOffset(-1);
     }
     removeVoidArgumentTokens(Result, SourceRange(Start, End),
                              "function definition");
-  } else {
-    removeVoidArgumentTokens(Result, Function->getSourceRange(),
+  } else
+    removeVoidArgumentTokens(Result, SourceRange(Start, End),
                              "function declaration");
-  }
+
+bool isMacroIdentifier(const IdentifierTable &Idents, const Token &ProtoToken) {
+  if (!ProtoToken.is(tok::TokenKind::raw_identifier))
+    return false;
+
+  IdentifierTable::iterator It = Idents.find(ProtoToken.getRawIdentifier());
+  if (It == Idents.end())
+    return false;
+
+  return It->second->hadMacroDefinition();
 }
 
 void RedundantVoidArgCheck::removeVoidArgumentTokens(
     const ast_matchers::MatchFinder::MatchResult &Result, SourceRange Range,
     StringRef GrammarLocation) {
   CharSourceRange CharRange =
       Lexer::makeFileCharRange(CharSourceRange::getTokenRange(Range),
                                *Result.SourceManager, getLangOpts());
 
   std::string DeclText =
       Lexer::getSourceText(CharRange, *Result.SourceManager, getLangOpts())
           .str();
   Lexer PrototypeLexer(CharRange.getBegin(), getLangOpts(), DeclText.data(),
                        DeclText.data(), DeclText.data() + DeclText.size());
-  enum TokenState {
-    NothingYet,
-    SawLeftParen,
-    SawVoid,
+  enum class TokenState {
+    Start,
+    MacroId,
+    MacroLeftParen,
+    MacroArguments,
+    LeftParen,
+    Void,
   };
-  TokenState State = NothingYet;
+  TokenState State = TokenState::Start;
   Token VoidToken;
   Token ProtoToken;
+  const IdentifierTable &Idents = Result.Context->Idents;
+  int MacroLevel = 0;
   std::string Diagnostic =
       ("redundant void argument list in " + GrammarLocation).str();
 
   while (!PrototypeLexer.LexFromRawLexer(ProtoToken)) {
     switch (State) {
-    case NothingYet:
-      if (ProtoToken.is(tok::TokenKind::l_paren)) {
-        State = SawLeftParen;
+    case TokenState::Start:
+      if (ProtoToken.is(tok::TokenKind::l_paren))
+        State = TokenState::LeftParen;
+      else if (isMacroIdentifier(Idents, ProtoToken))
+        State = TokenState::MacroId;
+      break;
+    case TokenState::MacroId:
+      if (ProtoToken.is(tok::TokenKind::l_paren))
+        State = TokenState::MacroLeftParen;
+      else
+        State = TokenState::Start;
+      break;
+    case TokenState::MacroLeftParen:
+      ++MacroLevel;
+      if (ProtoToken.is(tok::TokenKind::raw_identifier)) {
+        if (isMacroIdentifier(Idents, ProtoToken))
+          State = TokenState::MacroId;
+        else
+          State = TokenState::MacroArguments;
+      } else if (ProtoToken.is(tok::TokenKind::r_paren)) {
+        --MacroLevel;
+        if (MacroLevel == 0)
+          State = TokenState::Start;
+        else
+          State = TokenState::MacroId;
+      } else
+        State = TokenState::MacroArguments;
+      break;
+    case TokenState::MacroArguments:
+      if (isMacroIdentifier(Idents, ProtoToken))
+        State = TokenState::MacroLeftParen;
+      else if (ProtoToken.is(tok::TokenKind::r_paren)) {
+        --MacroLevel;
+        if (MacroLevel == 0)
+          State = TokenState::Start;
       }
       break;
-    case SawLeftParen:
-      if (ProtoToken.is(tok::TokenKind::raw_identifier) &&
-          ProtoToken.getRawIdentifier() == "void") {
-        State = SawVoid;
+    case TokenState::LeftParen:
+      if (ProtoToken.is(tok::TokenKind::raw_identifier)) {
+        if (isMacroIdentifier(Idents, ProtoToken))
+          State = TokenState::MacroId;
+        else if (ProtoToken.getRawIdentifier() == "void") {
+          State = TokenState::Void;
           VoidToken = ProtoToken;
-      } else if (ProtoToken.is(tok::TokenKind::l_paren)) {
-        State = SawLeftParen;
-      } else {
-        State = NothingYet;
         }
+      } else if (ProtoToken.is(tok::TokenKind::l_paren))
+        State = TokenState::LeftParen;
+      else
+        State = TokenState::Start;
       break;
-    case SawVoid:
-      State = NothingYet;
-      if (ProtoToken.is(tok::TokenKind::r_paren)) {
+    case TokenState::Void:
+      State = TokenState::Start;
+      if (ProtoToken.is(tok::TokenKind::r_paren))
         removeVoidToken(VoidToken, Diagnostic);
-      } else if (ProtoToken.is(tok::TokenKind::l_paren)) {
-        State = SawLeftParen;
-      }
+      else if (ProtoToken.is(tok::TokenKind::l_paren))
+        State = TokenState::LeftParen;
       break;
     }
   }
 
-  if (State == SawVoid && ProtoToken.is(tok::TokenKind::r_paren)) {
+  if (State == TokenState::Void && ProtoToken.is(tok::TokenKind::r_paren))
     removeVoidToken(VoidToken, Diagnostic);
-  }
-}
 
 void RedundantVoidArgCheck::removeVoidToken(Token VoidToken,
                                             StringRef Diagnostic) {
   SourceLocation VoidLoc = VoidToken.getLocation();
   diag(VoidLoc, Diagnostic) << FixItHint::CreateRemoval(VoidLoc);
 }
 
 void RedundantVoidArgCheck::processTypedefNameDecl(
     const MatchFinder::MatchResult &Result,
     const TypedefNameDecl *TypedefName) {
-  if (protoTypeHasNoParms(TypedefName->getUnderlyingType())) {
+  if (protoTypeHasNoParms(TypedefName->getUnderlyingType()))
     removeVoidArgumentTokens(Result, TypedefName->getSourceRange(),
                              isa<TypedefDecl>(TypedefName) ? "typedef"
                                                            : "type alias");
-  }
-}
 
 void RedundantVoidArgCheck::processFieldDecl(
     const MatchFinder::MatchResult &Result, const FieldDecl *Member) {
-  if (protoTypeHasNoParms(Member->getType())) {
+  if (protoTypeHasNoParms(Member->getType()))
     removeVoidArgumentTokens(Result, Member->getSourceRange(),
                              "field declaration");
-  }
-}
 
 void RedundantVoidArgCheck::processVarDecl(
     const MatchFinder::MatchResult &Result, const VarDecl *Var) {
   if (protoTypeHasNoParms(Var->getType())) {
     SourceLocation Begin = Var->getBeginLoc();
     if (Var->hasInit()) {
       SourceLocation InitStart =
           Result.SourceManager->getExpansionLoc(Var->getInit()->getBeginLoc())
               .getLocWithOffset(-1);
       removeVoidArgumentTokens(Result, SourceRange(Begin, InitStart),
                                "variable declaration with initializer");
-    } else {
+    } else
       removeVoidArgumentTokens(Result, Var->getSourceRange(),
                                "variable declaration");
-    }
-  }
-}
 
 void RedundantVoidArgCheck::processNamedCastExpr(
     const MatchFinder::MatchResult &Result, const CXXNamedCastExpr *NamedCast) {
-  if (protoTypeHasNoParms(NamedCast->getTypeAsWritten())) {
+  if (protoTypeHasNoParms(NamedCast->getTypeAsWritten()))
     removeVoidArgumentTokens(
         Result,
         NamedCast->getTypeInfoAsWritten()->getTypeLoc().getSourceRange(),
         "named cast");
-  }
-}
 
 void RedundantVoidArgCheck::processExplicitCastExpr(
     const MatchFinder::MatchResult &Result,
     const ExplicitCastExpr *ExplicitCast) {
-  if (protoTypeHasNoParms(ExplicitCast->getTypeAsWritten())) {
+  if (protoTypeHasNoParms(ExplicitCast->getTypeAsWritten()))
     removeVoidArgumentTokens(Result, ExplicitCast->getSourceRange(),
                              "cast expression");
-  }
-}
 
 void RedundantVoidArgCheck::processLambdaExpr(
     const MatchFinder::MatchResult &Result, const LambdaExpr *Lambda) {
   if (Lambda->getLambdaClass()->getLambdaCallOperator()->getNumParams() == 0 &&
       Lambda->hasExplicitParameters()) {
     SourceManager *SM = Result.SourceManager;
     TypeLoc TL = Lambda->getLambdaClass()->getLambdaTypeInfo()->getTypeLoc();
     removeVoidArgumentTokens(Result,

clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp

typedef void (function_ptr2)
     );
 // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: {{.*}} in typedef
 // CHECK-FIXES:      {{^typedef void \(function_ptr2\)$}}
 // CHECK-FIXES-NEXT: {{^    \($}}
 // CHECK-FIXES-NEXT: {{^        $}}
 // CHECK-FIXES-NEXT: {{^    \);$}}
 
 // intentionally not LLVM style to check preservation of whitespace
+// clang-format off
 typedef
 void
 (
 *
 (
 *
 returns_fn_returns_fn_void_void_t2
 (

 // CHECK-FIXES-NEXT: {{^\($}}
 // CHECK-FIXES-NOT:  {{[^ ]}}
 // CHECK-FIXES:      {{^\)$}}
 // CHECK-FIXES-NEXT: {{^\)$}}
 // CHECK-FIXES-NEXT: {{^\($}}
 // CHECK-FIXES-NOT:  {{[^ ]}}
 // CHECK-FIXES:      {{^\)$}}
 // CHECK-FIXES-NEXT: {{^;$}}
-
+// clang-format on
 
 void (gronk::*p1)(void);
 // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: {{.*}} in variable declaration
 // CHECK-FIXES: {{^}}void (gronk::*p1)();{{$}}
 
 void (gronk::*p2)(void) = &gronk::foo;
 // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: {{.*}} in variable declaration with initializer
 // CHECK-FIXES: {{^}}void (gronk::*p2)() = &gronk::foo;{{$}}

 
 //Template instantiation
 void f_testTemplate() {
   S_1();
   S_2<int>();
   S_3<int>();
   g_3<int>();
 }
+
+#define return_t(T) T
+extern return_t(void) func(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: redundant void argument list in function declaration
+// CHECK-FIXES: extern return_t(void) func();
+
+return_t(void) func(void) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: redundant void argument list in function definition
+  // CHECK-FIXES: return_t(void) func() {
+  int a;
+  (void)a;
+}
+
+extern return_t(void) func(return_t(void) (*fp)(void));
+// CHECK-MESSAGES: :[[@LINE-1]]:49: warning: redundant void argument list in variable declaration
+// CHECK-FIXES: extern return_t(void) func(return_t(void) (*fp)());
+
+return_t(void) func(return_t(void) (*fp)(void)) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: redundant void argument list in variable declaration
+  // CHECK-FIXES: return_t(void) func(return_t(void) (*fp)()) {
+  int a;
+  (void)a;
+}
+
+extern return_t(return_t(void)) func2(return_t(return_t(void)) (*fp)(void));
+// CHECK-MESSAGES: :[[@LINE-1]]:70: warning: redundant void argument list in variable declaration
+// CHECK-FIXES: extern return_t(return_t(void)) func2(return_t(return_t(void)) (*fp)());
+
+return_t(return_t(void)) func2(return_t(return_t(void)) (*fp)(void)) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:63: warning: redundant void argument list in variable declaration
+  // CHECK-FIXES: return_t(return_t(void)) func2(return_t(return_t(void)) (*fp)()) {
+  int a;
+  (void)a;
+}
+#undef return_t