trixirt updated this revision to Diff 308214.
trixirt edited the summary of this revision.
trixirt added a comment.
Refactor to combine switch and trailing macro into one fixer
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91789/new/
https://reviews.llvm.org/D91789
Files:
clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt
clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.cpp
clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h
clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
clang-tools-extra/test/clang-tidy/checkers/linuxkernel-macro-trailing-semi.c
clang-tools-extra/test/clang-tidy/checkers/linuxkernel-switch-semi.c
Index: clang-tools-extra/test/clang-tidy/checkers/linuxkernel-switch-semi.c
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/linuxkernel-switch-semi.c
@@ -0,0 +1,41 @@
+// RUN: %check_clang_tidy %s linuxkernel-extra-semi %t -- \
+// RUN: -config="{CheckOptions: \
+// RUN: [{key: linuxkernel-extra-semi.Fixer, \
+// RUN: value: 'Switch'}, \
+// RUN: ]}"
+
+int f(int a) {
+ switch (a) {
+ case 1:
+ return 0;
+ default:
+ break;
+ };
+ // CHECK-MESSAGES: :[[@LINE-1]]:4: warning: unneeded semicolon
+ // CHECK-FIXES: }{{$}}
+ return 0;
+}
+
+// A normal switch, should not produce a warning
+int p1(int a) {
+ switch (a) {
+ case 1:
+ return 0;
+ default:
+ break;
+ }
+ return 0;
+}
+
+#define EMPTY_MACRO()
+// A corner case, do not fix an empty macro
+int p2(int a) {
+ switch (a) {
+ case 1:
+ return 0;
+ default:
+ break;
+ }
+ EMPTY_MACRO();
+ return 0;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/linuxkernel-macro-trailing-semi.c
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/linuxkernel-macro-trailing-semi.c
@@ -0,0 +1,32 @@
+// RUN: %check_clang_tidy %s linuxkernel-extra-semi %t -- \
+// RUN: -config="{CheckOptions: \
+// RUN: [{key: linuxkernel-extra-semi.Fixer, \
+// RUN: value: 'TrailingMacro'}, \
+// RUN: ]}"
+
+#define M(a) a++;
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: unneeded semicolon
+// CHECK-FIXES: #define M(a) a++{{$}}
+
+int f() {
+ int v = 0;
+ M(v);
+ return v;
+}
+
+// A corner case
+// An empty macro could conditionally contain another path so
+// do not warn or fix.
+#ifdef UNSET_CONDITION
+#define E() ;
+#else
+#define E()
+#endif
+#define N(a) a++;
+
+int g() {
+ int v = 0;
+ N(v)
+ E();
+ return v;
+}
Index: clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
+++ clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
@@ -9,6 +9,7 @@
#include "../ClangTidy.h"
#include "../ClangTidyModule.h"
#include "../ClangTidyModuleRegistry.h"
+#include "ExtraSemiCheck.h"
#include "MustCheckErrsCheck.h"
namespace clang {
@@ -19,6 +20,7 @@
class LinuxKernelModule : public ClangTidyModule {
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+ CheckFactories.registerCheck<ExtraSemiCheck>("linuxkernel-extra-semi");
CheckFactories.registerCheck<MustCheckErrsCheck>(
"linuxkernel-must-check-errs");
}
Index: clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h
@@ -0,0 +1,43 @@
+//===--- ExtraSemiCheck.h - clang-tidy --------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_EXTRASEMICHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_EXTRASEMICHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/Lex/MacroInfo.h"
+#include <vector>
+
+namespace clang {
+namespace tidy {
+namespace linuxkernel {
+
+enum ExtraSemiFixerKind { ESFK_None, ESFK_Switch, ESFK_TrailingMacro };
+class ExtraSemiCheck : public ClangTidyCheck {
+public:
+ ExtraSemiCheck(StringRef Name, ClangTidyContext *Context);
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+ Preprocessor *ModuleExpanderPP) override final;
+
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void checkMacro(SourceManager &sourceMgr, const Token &MacroNameTok,
+ const MacroInfo *MI);
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+ std::vector<const MacroInfo *> SuspectMacros;
+ enum ExtraSemiFixerKind FixerKind;
+ const std::string ExtraSemiFixerKindName;
+};
+
+} // namespace linuxkernel
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_EXTRASEMI_CHECK_H
Index: clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.cpp
@@ -0,0 +1,205 @@
+//===--- ExtraSemiCheck.cpp - clang-tidy ----------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ExtraSemiCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace linuxkernel {
+
+class ExtraSemiCheckPPCallbacks : public PPCallbacks {
+public:
+ ExtraSemiCheckPPCallbacks(Preprocessor *PP, ExtraSemiCheck *Check)
+ : PP(PP), Check(Check) {}
+
+ void MacroDefined(const Token &MacroNameTok,
+ const MacroDirective *MD) override {
+ const MacroInfo *MI = MD->getMacroInfo();
+
+ if (MI->isBuiltinMacro() || MI->isObjectLike())
+ return;
+ Check->checkMacro(PP->getSourceManager(), MacroNameTok, MI);
+ }
+
+private:
+ Preprocessor *PP;
+ ExtraSemiCheck *Check;
+};
+
+ExtraSemiCheck::ExtraSemiCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context), FixerKind(ESFK_None),
+ ExtraSemiFixerKindName(Options.get("Fixer", ExtraSemiFixerKindName)) {
+ if (ExtraSemiFixerKindName == "Switch")
+ FixerKind = ESFK_Switch;
+ else if (ExtraSemiFixerKindName == "TrailingMacro")
+ FixerKind = ESFK_TrailingMacro;
+}
+
+void ExtraSemiCheck::registerMatchers(MatchFinder *Finder) {
+ if (FixerKind == ESFK_Switch) {
+ // From the reproducer
+ // void foo (int a) {
+ // switch (a) {};
+ // }
+ // The AST
+ // `-FunctionDecl
+ // |-ParmVarDecl
+ // `-CompoundStmt <--- "comp", 'C'
+ // |-SwitchStmt <-- "switch", 'S'
+ // | |-ImplicitCastExpr
+ // | | `-DeclRefExpr
+ // | `-CompoundStmt
+ // `-NullStmt <-------------- 'N'
+ Finder->addMatcher(
+ compoundStmt(has(switchStmt().bind("switch"))).bind("comp"), this);
+ } else if (FixerKind == ESFK_TrailingMacro) {
+ // From the reproducer
+ // #define M(a) a++;
+ // int f() {
+ // int v = 0;
+ // M(v);
+ // return v;
+ // }
+ // The AST
+ // `-FunctionDecl
+ // `-CompoundStmt <--- "comp", 'C'
+ // ...
+ // |-UnaryOperator
+ // | `-DeclRefExpr <-------- 'E'
+ // |-NullStmt <------ "null" 'N'
+ // ...
+ Finder->addMatcher(compoundStmt(has(nullStmt().bind("null"))).bind("comp"),
+ this);
+ }
+}
+
+void ExtraSemiCheck::registerPPCallbacks(const SourceManager &SM,
+ Preprocessor *PP,
+ Preprocessor *ModuleExpanderPP) {
+ if (FixerKind == ESFK_TrailingMacro) {
+ ModuleExpanderPP->addPPCallbacks(
+ std::make_unique<ExtraSemiCheckPPCallbacks>(ModuleExpanderPP, this));
+ }
+}
+
+void ExtraSemiCheck::check(const MatchFinder::MatchResult &Result) {
+ if (FixerKind == ESFK_Switch) {
+ const auto *C = Result.Nodes.getNodeAs<CompoundStmt>("comp");
+ const auto *S = Result.Nodes.getNodeAs<SwitchStmt>("switch");
+
+ auto Current = C->body_begin();
+ auto Next = Current;
+ Next++;
+ while (Next != C->body_end()) {
+ if (*Current == S) {
+ if (const auto *N = dyn_cast<NullStmt>(*Next)) {
+ // This code has the same AST as the reproducer
+ //
+ // #define EMPTY()
+ // void foo (int a) {
+ // switch (a) {} EMPTY();
+ // }
+ //
+ // But we do not want to remove the ; because the
+ // macro may only be conditionally empty.
+ // ex/ the release version of a debug macro
+ //
+ // So check that the NullStmt does not have a
+ // leading empty macro.
+ if (!N->hasLeadingEmptyMacro() &&
+ S->getBody()->getEndLoc().isValid() &&
+ N->getSemiLoc().isValid()) {
+ auto H = FixItHint::CreateReplacement(
+ SourceRange(S->getBody()->getEndLoc(), N->getSemiLoc()), "}");
+ diag(N->getSemiLoc(), "unneeded semicolon") << H;
+ break;
+ }
+ }
+ }
+ Current = Next;
+ Next++;
+ }
+ } else if (FixerKind == ESFK_TrailingMacro) {
+ const auto *C = Result.Nodes.getNodeAs<CompoundStmt>("comp");
+ const auto *N = Result.Nodes.getNodeAs<NullStmt>("null");
+
+ auto Current = C->body_begin();
+ auto Next = Current;
+ Next++;
+ while (Next != C->body_end()) {
+
+ if (*Next == N) {
+ // This code has the same AST as the reproducer
+ //
+ // #define NOT_EMPTY(a) a++;
+ // #define EMPTY()
+ // void foo (int a) {
+ // NOT_EMPTY(a);
+ // EMPTY();
+ // }
+ //
+ // But we do not want to remove the ; because the
+ // macro may only be conditionally empty.
+ // ex/ the release version of a debug macro
+ //
+ // So check that the NullStmt does not have a
+ // leading empty macro.
+ if (!N->hasLeadingEmptyMacro() && N->getEndLoc().isValid()) {
+ if (const auto *E = dyn_cast<Expr>(*Current)) {
+ SourceLocation Loc = E->getEndLoc();
+ if (Loc.isMacroID()) {
+ SourceManager &SM = Result.Context->getSourceManager();
+ FullSourceLoc SpellingLoc =
+ FullSourceLoc(Loc, SM).getSpellingLoc();
+
+ for (const MacroInfo *MI : SuspectMacros) {
+ auto TI = MI->tokens_begin();
+ for (; TI != MI->tokens_end(); TI++) {
+ SourceLocation L = TI->getLocation();
+ if (SpellingLoc == L)
+ break;
+ }
+ if (TI != MI->tokens_end()) {
+ const Token &Tok =
+ MI->getReplacementToken(MI->getNumTokens() - 1);
+ SourceLocation FixLoc = Tok.getLocation();
+ SourceLocation EndLoc = Tok.getEndLoc();
+ diag(FixLoc, "unneeded semicolon")
+ << FixItHint::CreateRemoval(SourceRange(FixLoc, EndLoc));
+ break;
+ }
+ }
+ }
+ }
+ }
+ }
+ Current = Next;
+ Next++;
+ }
+ }
+}
+void ExtraSemiCheck::checkMacro(SourceManager &SM, const Token &MacroNameTok,
+ const MacroInfo *MI) {
+ unsigned num = MI->getNumTokens();
+ if (num && MI->getReplacementToken(num - 1).is(tok::semi))
+ SuspectMacros.push_back(MI);
+}
+
+void ExtraSemiCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "Fixer", "");
+}
+
+} // namespace linuxkernel
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt
@@ -4,6 +4,7 @@
)
add_clang_library(clangTidyLinuxKernelModule
+ ExtraSemiCheck.cpp
LinuxKernelTidyModule.cpp
MustCheckErrsCheck.cpp
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
