-
Notifications
You must be signed in to change notification settings - Fork 15.6k
[clang-tidy] Refactor modernize-redundant-void-arg
#173340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Victor Chernyakin (localspook) ChangesThis refactor is not entirely NFC:
Patch is 44.67 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/173340.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp b/clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
index 831c8565eb74d..01f6ff60b9d6e 100644
--- a/clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
@@ -7,281 +7,50 @@
//===----------------------------------------------------------------------===//
#include "RedundantVoidArgCheck.h"
-#include "clang/Frontend/CompilerInstance.h"
-#include "clang/Lex/Lexer.h"
+#include "../utils/LexerUtils.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
using namespace clang::ast_matchers;
+using namespace clang::ast_matchers::internal;
namespace clang::tidy::modernize {
-// Determine if the given QualType is a nullary function or pointer to same.
-static bool protoTypeHasNoParms(QualType QT) {
- if (const auto *PT = QT->getAs<PointerType>())
- QT = PT->getPointeeType();
- if (auto *MPT = QT->getAs<MemberPointerType>())
- QT = MPT->getPointeeType();
- if (const auto *FP = QT->getAs<FunctionProtoType>())
- return FP->getNumParams() == 0;
- return false;
-}
-
-static const char FunctionId[] = "function";
-static const char TypedefId[] = "typedef";
-static const char FieldId[] = "field";
-static const char VarId[] = "var";
-static const char NamedCastId[] = "named-cast";
-static const char CStyleCastId[] = "c-style-cast";
-static const char ExplicitCastId[] = "explicit-cast";
-static const char LambdaId[] = "lambda";
-
void RedundantVoidArgCheck::registerMatchers(MatchFinder *Finder) {
- Finder->addMatcher(functionDecl(parameterCountIs(0), unless(isImplicit()),
- unless(isInstantiated()), unless(isExternC()))
- .bind(FunctionId),
- this);
- Finder->addMatcher(typedefNameDecl(unless(isImplicit())).bind(TypedefId),
+ const VariadicDynCastAllOfMatcher<TypeLoc, FunctionProtoTypeLoc>
+ functionProtoTypeLoc; // NOLINT(readability-identifier-naming)
+ Finder->addMatcher(traverse(TK_IgnoreUnlessSpelledInSource,
+ functionProtoTypeLoc(
+ unless(hasParent(functionDecl(isExternC()))))
+ .bind("fn")),
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);
+ traverse(TK_IgnoreUnlessSpelledInSource, lambdaExpr().bind("fn")), this);
}
void RedundantVoidArgCheck::check(const MatchFinder::MatchResult &Result) {
- const BoundNodes &Nodes = Result.Nodes;
- if (const auto *Function = Nodes.getNodeAs<FunctionDecl>(FunctionId))
- processFunctionDecl(Result, Function);
- else if (const auto *TypedefName =
- Nodes.getNodeAs<TypedefNameDecl>(TypedefId))
- processTypedefNameDecl(Result, TypedefName);
- else if (const auto *Member = Nodes.getNodeAs<FieldDecl>(FieldId))
- processFieldDecl(Result, Member);
- else if (const auto *Var = Nodes.getNodeAs<VarDecl>(VarId))
- processVarDecl(Result, Var);
- else if (const auto *NamedCast =
- Nodes.getNodeAs<CXXNamedCastExpr>(NamedCastId))
- processNamedCastExpr(Result, NamedCast);
- else if (const auto *CStyleCast =
- Nodes.getNodeAs<CStyleCastExpr>(CStyleCastId))
- processExplicitCastExpr(Result, CStyleCast);
- else if (const auto *ExplicitCast =
- Nodes.getNodeAs<ExplicitCastExpr>(ExplicitCastId))
- processExplicitCastExpr(Result, ExplicitCast);
- else if (const auto *Lambda = Nodes.getNodeAs<LambdaExpr>(LambdaId))
- processLambdaExpr(Result, Lambda);
-}
-
-void RedundantVoidArgCheck::processFunctionDecl(
- const MatchFinder::MatchResult &Result, const FunctionDecl *Function) {
- const auto *Method = dyn_cast<CXXMethodDecl>(Function);
- const SourceLocation Start = Method && Method->getParent()->isLambda()
- ? Method->getBeginLoc()
- : Function->getLocation();
- SourceLocation End = Function->getEndLoc();
- 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, SourceRange(Start, End),
- "function declaration");
-}
-
-static bool isMacroIdentifier(const IdentifierTable &Idents,
- const Token &ProtoToken) {
- if (!ProtoToken.is(tok::TokenKind::raw_identifier))
- return false;
-
- const 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) {
- const 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 class TokenState {
- Start,
- MacroId,
- MacroLeftParen,
- MacroArguments,
- LeftParen,
- Void,
- };
- TokenState State = TokenState::Start;
- Token VoidToken;
- Token ProtoToken;
- const IdentifierTable &Idents = Result.Context->Idents;
- int MacroLevel = 0;
- const std::string Diagnostic =
- ("redundant void argument list in " + GrammarLocation).str();
-
- while (!PrototypeLexer.LexFromRawLexer(ProtoToken)) {
- switch (State) {
- 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 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 = TokenState::LeftParen;
- else
- State = TokenState::Start;
- break;
- 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 = TokenState::LeftParen;
- break;
- }
- }
-
- if (State == TokenState::Void && ProtoToken.is(tok::TokenKind::r_paren))
- removeVoidToken(VoidToken, Diagnostic);
-}
-
-void RedundantVoidArgCheck::removeVoidToken(Token VoidToken,
- StringRef Diagnostic) {
- const SourceLocation VoidLoc = VoidToken.getLocation();
- diag(VoidLoc, Diagnostic) << FixItHint::CreateRemoval(VoidLoc);
-}
-
-void RedundantVoidArgCheck::processTypedefNameDecl(
- const MatchFinder::MatchResult &Result,
- const TypedefNameDecl *TypedefName) {
- 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()))
- removeVoidArgumentTokens(Result, Member->getSourceRange(),
- "field declaration");
-}
-
-void RedundantVoidArgCheck::processVarDecl(
- const MatchFinder::MatchResult &Result, const VarDecl *Var) {
- if (protoTypeHasNoParms(Var->getType())) {
- const SourceLocation Begin = Var->getBeginLoc();
- if (Var->hasInit()) {
- const SourceLocation InitStart =
- Result.SourceManager->getExpansionLoc(Var->getInit()->getBeginLoc())
- .getLocWithOffset(-1);
- removeVoidArgumentTokens(Result, SourceRange(Begin, InitStart),
- "variable declaration with initializer");
- } else
- removeVoidArgumentTokens(Result, Var->getSourceRange(),
- "variable declaration");
- }
-}
-
-void RedundantVoidArgCheck::processNamedCastExpr(
- const MatchFinder::MatchResult &Result, const CXXNamedCastExpr *NamedCast) {
- 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()))
- removeVoidArgumentTokens(Result, ExplicitCast->getSourceRange(),
- "cast expression");
-}
-
-void RedundantVoidArgCheck::processLambdaExpr(
- const MatchFinder::MatchResult &Result, const LambdaExpr *Lambda) {
- if (Lambda->getLambdaClass()->getLambdaCallOperator()->getNumParams() == 0 &&
- Lambda->hasExplicitParameters()) {
- const SourceManager *SM = Result.SourceManager;
- const TypeLoc TL =
- Lambda->getLambdaClass()->getLambdaTypeInfo()->getTypeLoc();
- removeVoidArgumentTokens(Result,
- {SM->getSpellingLoc(TL.getBeginLoc()),
- SM->getSpellingLoc(TL.getEndLoc())},
- "lambda expression");
- }
+ const FunctionProtoTypeLoc TL = [&] {
+ if (const auto *TL = Result.Nodes.getNodeAs<FunctionProtoTypeLoc>("fn"))
+ return *TL;
+ return Result.Nodes.getNodeAs<LambdaExpr>("fn")
+ ->getCallOperator()
+ ->getTypeSourceInfo()
+ ->getTypeLoc()
+ .getAs<FunctionProtoTypeLoc>();
+ }();
+
+ if (TL.getNumParams() != 0)
+ return;
+
+ const std::optional<Token> Tok = utils::lexer::findNextTokenSkippingComments(
+ Result.SourceManager->getSpellingLoc(TL.getLParenLoc()),
+ *Result.SourceManager, getLangOpts());
+
+ if (!Tok || Tok->isNot(tok::raw_identifier) ||
+ Tok->getRawIdentifier() != "void")
+ return;
+
+ diag(Tok->getLocation(), "redundant void argument list")
+ << FixItHint::CreateRemoval(Tok->getLocation());
}
} // namespace clang::tidy::modernize
diff --git a/clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.h b/clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.h
index d6edd9950ddae..77ebdc84be7e9 100644
--- a/clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.h
+++ b/clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.h
@@ -10,9 +10,6 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REDUNDANTVOIDARGCHECK_H
#include "../ClangTidyCheck.h"
-#include "clang/Lex/Token.h"
-
-#include <string>
namespace clang::tidy::modernize {
@@ -38,37 +35,6 @@ class RedundantVoidArgCheck : public ClangTidyCheck {
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
-
-private:
- void processFunctionDecl(const ast_matchers::MatchFinder::MatchResult &Result,
- const FunctionDecl *Function);
-
- void
- processTypedefNameDecl(const ast_matchers::MatchFinder::MatchResult &Result,
- const TypedefNameDecl *Typedef);
-
- void processFieldDecl(const ast_matchers::MatchFinder::MatchResult &Result,
- const FieldDecl *Member);
-
- void processVarDecl(const ast_matchers::MatchFinder::MatchResult &Result,
- const VarDecl *Var);
-
- void
- processNamedCastExpr(const ast_matchers::MatchFinder::MatchResult &Result,
- const CXXNamedCastExpr *NamedCast);
-
- void
- processExplicitCastExpr(const ast_matchers::MatchFinder::MatchResult &Result,
- const ExplicitCastExpr *ExplicitCast);
-
- void processLambdaExpr(const ast_matchers::MatchFinder::MatchResult &Result,
- const LambdaExpr *Lambda);
-
- void
- removeVoidArgumentTokens(const ast_matchers::MatchFinder::MatchResult &Result,
- SourceRange Range, StringRef GrammarLocation);
-
- void removeVoidToken(Token VoidToken, StringRef Diagnostic);
};
} // namespace clang::tidy::modernize
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg-delayed.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg-delayed.cpp
index 4e05ada1d2992..b5916146ac8d8 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg-delayed.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg-delayed.cpp
@@ -1,7 +1,7 @@
// RUN: %check_clang_tidy %s modernize-redundant-void-arg %t -- -- -fdelayed-template-parsing
int foo(void) {
-// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant void argument list in function definition [modernize-redundant-void-arg]
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant void argument list [modernize-redundant-void-arg]
// CHECK-FIXES: int foo() {
return 0;
}
@@ -9,7 +9,7 @@ int foo(void) {
template <class T>
struct MyFoo {
int foo(void) {
-// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: redundant void argument list in function definition [modernize-redundant-void-arg]
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: redundant void argument list [modernize-redundant-void-arg]
// CHECK-FIXES: int foo() {
return 0;
}
@@ -21,7 +21,7 @@ template <class T>
struct MyBar {
// This declaration isn't instantiated and won't be parsed 'delayed-template-parsing'.
int foo(void) {
-// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: redundant void argument list in function definition [modernize-redundant-void-arg]
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: redundant void argument list [modernize-redundant-void-arg]
// CHECK-FIXES: int foo() {
return 0;
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg.cpp
index 631149761e5e8..09dc2bb5c1775 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg.cpp
@@ -18,7 +18,7 @@ extern int i;
int j = 1;
int foo(void) {
-// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant void argument list in function definition [modernize-redundant-void-arg]
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant void argument list [modernize-redundant-void-arg]
// CHECK-FIXES: int foo() {
return 0;
}
@@ -30,24 +30,24 @@ typedef void my_void;
// A function taking void and returning a pointer to function taking void
// and returning int.
int (*returns_fn_void_int(void))(void);
-// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: {{.*}} in function declaration
-// CHECK-MESSAGES: :[[@LINE-2]]:34: warning: {{.*}} in function declaration
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: redundant void argument list [modernize-redundant-void-arg]
+// CHECK-MESSAGES: :[[@LINE-2]]:34: warning: redundant void argument list [modernize-redundant-void-arg]
// CHECK-FIXES: int (*returns_fn_void_int())();
typedef int (*returns_fn_void_int_t(void))(void);
-// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: {{.*}} in typedef
-// CHECK-MESSAGES: :[[@LINE-2]]:44: warning: {{.*}} in typedef
+// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: redundant void argument list [modernize-redundant-void-arg]
+// CHECK-MESSAGES: :[[@LINE-2]]:44: warning: redundant void argument list [modernize-redundant-void-arg]
// CHECK-FIXES: typedef int (*returns_fn_void_int_t())();
// Should work for type aliases as well as typedef.
using returns_fn_void_int_t2 = int (*(void))(void);
-// CHECK-MESSAGES: :[[@LINE-1]]:39: warning: {{.*}} in type alias
-// CHECK-MESSAGES: :[[@LINE-2]]:46: warning: {{.*}} in type alias
+// CHECK-MESSAGES: :[[@LINE-1]]:39: warning: redundant void argument list [modernize-redundant-void-arg]
+// CHECK-MESSAGES: :[[@LINE-2]]:46: warning: redundant void argument list [modernize-redundant-void-arg]
// CHECK-FIXES: using returns_fn_void_int_t2 = int (*())();
int (*returns_fn_void_int(void))(void) {
-// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: {{.*}} in function definition
-// CHECK-MESSAGES: :[[@LINE-2]]:34: warning: {{.*}} in function definition
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: redundant void argument list [modernize-redundant-void-arg]
+// CHECK-MESSAGES: :[[@LINE-2]]:34: warning: redundant void argument list [modernize-redundant-void-arg]
// CHECK-FIXES: int (*returns_fn_void_int())() {
return nullptr;
}
@@ -55,27 +55,27 @@ int (*returns_fn_void_int(void))(void) {
// A function taking void and returning a pointer to a function taking void
// and returning a pointer to a function taking void and returning void.
void (*(*returns_fn_returns_fn_void_void(void))(void))(void);
-// CHECK-MESSAGES: :[[@LINE-1]]:42: warning: {{.*}} in function declaration
-// CHECK-MESSAGES: :[[@LINE-2]]:49: warning: {{.*}} in function declaration
-// CHECK-MESSAGES: :[[@LINE-3]]:56: warning: {{.*}} in function declaration
+// CHECK-MESSAGES: :[[@LINE-1]]:42: warning: redundant void argument list [modernize-redundant-void-arg]
+// CHECK-MESSAGES: :[[@LINE-2]]:49: warning: redundant void argument list [modernize-redundant-void-arg]
+// CHECK-MESSAGES: :[[@LINE-3]]:56: warning: redundant void argument list [modernize-redundant-void-arg]
// CHECK-FIXES: void (*(*returns_fn_returns_fn_void_void())())();
typedef void (*(*returns_fn_returns_fn_void_void_t(void))(void))(void);
-// CHECK-MESSAGES: :[[@LINE-1]]:52: warning: {{...
[truncated]
|
zeyi2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this change is not strictly NFC (the diagnostic message has changed, which is user-visible), should we document it in ReleaseNotes.rst?
|
My instinct is no, because I don't think the change is meaningful enough that a user would care about it. |
34618f9 to
ccd5884
Compare
|
Please add the matcher in a separate PR. See #168990 for adding a matcher in ASTMatchers.h. |
This refactor is not entirely NFC:
redundant void argument list in {function definition, lambda expression, etc.}is now justredundant void argument list. I don't think we're losing any important information though.voids in this test case:llvm-project/clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg.cpp
Lines 434 to 442 in 01effcd
Seeing as this check does want to remove
voidin macros, I believe this is fixing an FN. And I don't believe I'm violating the intent of the test case, because the intent was just to ensure the check doesn't crash when encountering it.