[clang-tidy] add 'IgnoreMarcos' option to 'avoid-goto' check

[vbvictor]

No description provided.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Baranov Victor (vbvictor)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/143554.diff

4 Files Affected:

• (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp (+16-1)
• (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidGotoCheck.h (+5-2)
• (modified) clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-goto.rst (+9)
• (modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-goto.cpp (+42-8)

diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
index 8ffa44d41fa98..00864b1d69618 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
@@ -17,8 +17,20 @@ namespace {
 AST_MATCHER(GotoStmt, isForwardJumping) {
   return Node.getBeginLoc() < Node.getLabel()->getBeginLoc();
 }
+
+AST_MATCHER(GotoStmt, isInMacro) {
+  return Node.getBeginLoc().isMacroID() && Node.getEndLoc().isMacroID();
+}
 } // namespace
 
+AvoidGotoCheck::AvoidGotoCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IgnoreMacros(Options.get("IgnoreMacros", true)) {}
+
+void AvoidGotoCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnoreMacros", IgnoreMacros);
+}
+
 void AvoidGotoCheck::registerMatchers(MatchFinder *Finder) {
   // TODO: This check does not recognize `IndirectGotoStmt` which is a
   // GNU extension. These must be matched separately and an AST matcher
@@ -29,7 +41,10 @@ void AvoidGotoCheck::registerMatchers(MatchFinder *Finder) {
   auto Loop = mapAnyOf(forStmt, cxxForRangeStmt, whileStmt, doStmt);
   auto NestedLoop = Loop.with(hasAncestor(Loop));
 
-  Finder->addMatcher(gotoStmt(anyOf(unless(hasAncestor(NestedLoop)),
+  const ast_matchers::internal::Matcher<GotoStmt> Anything = anything();
+
+  Finder->addMatcher(gotoStmt(IgnoreMacros ? unless(isInMacro()) : Anything,
+                              anyOf(unless(hasAncestor(NestedLoop)),
                                     unless(isForwardJumping())))
                          .bind("goto"),
                      this);
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidGotoCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
index 883ba78855e7c..8eae409462c91 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
@@ -20,13 +20,16 @@ namespace clang::tidy::cppcoreguidelines {
 /// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-goto.html
 class AvoidGotoCheck : public ClangTidyCheck {
 public:
-  AvoidGotoCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  AvoidGotoCheck(StringRef Name, ClangTidyContext *Context);
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const bool IgnoreMacros;
 };
 
 } // namespace clang::tidy::cppcoreguidelines
diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-goto.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-goto.rst
index 71b579a4ae99e..823b3c376e6d0 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-goto.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-goto.rst
@@ -50,3 +50,12 @@ Modern C++ needs ``goto`` only to jump out of nested loops.
   some_operation();
 
 All other uses of ``goto`` are diagnosed in `C++`.
+
+
+Options
+-------
+
+.. option:: IgnoreMacros
+
+   If set to `true`, the check will not warn if both label and ``goto``
+   statement are placed inside a macro. Default is `true`.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-goto.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-goto.cpp
index ee236bc44695a..9a95b21cf3411 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-goto.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-goto.cpp
@@ -1,12 +1,13 @@
 // RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+// RUN: %check_clang_tidy -check-suffix=,MACRO %s cppcoreguidelines-avoid-goto %t -- -config="{CheckOptions: { cppcoreguidelines-avoid-goto.IgnoreMacros: false }}"
 
 void noop() {}
 
 int main() {
   noop();
   goto jump_to_me;
-  // CHECK-NOTES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
-  // CHECK-NOTES: [[@LINE+3]]:1: note: label defined here
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE+3]]:1: note: label defined here
   noop();
 
 jump_to_me:;
@@ -14,14 +15,14 @@ jump_to_me:;
 jump_backwards:;
   noop();
   goto jump_backwards;
-  // CHECK-NOTES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
-  // CHECK-NOTES: [[@LINE-4]]:1: note: label defined here
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-4]]:1: note: label defined here
 
   goto jump_in_line;
   ;
 jump_in_line:;
-  // CHECK-NOTES: [[@LINE-3]]:3: warning: avoid using 'goto' for flow control
-  // CHECK-NOTES: [[@LINE-2]]:1: note: label defined here
+  // CHECK-MESSAGES: [[@LINE-3]]:3: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-2]]:1: note: label defined here
 
   // Test the GNU extension https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html
 some_label:;
@@ -132,8 +133,41 @@ void jump_out_backwards() {
     for (int j = 0; j < 10; ++j) {
       if (i * j > 80)
         goto before_the_loop;
-      // CHECK-NOTES: [[@LINE-1]]:9: warning: avoid using 'goto' for flow control
-      // CHECK-NOTES: [[@LINE-8]]:1: note: label defined here
+      // CHECK-MESSAGES: [[@LINE-1]]:9: warning: avoid using 'goto' for flow control
+      // CHECK-MESSAGES: [[@LINE-8]]:1: note: label defined here
     }
   }
 }
+
+#define macro_goto_code \
+  noop(); \
+  goto jump_to_me; \
+  noop(); \
+jump_to_me:; \
+
+#define macro_goto_label jump_to_me:;
+#define macro_goto_jump goto jump_to_me;
+
+void inside_macro_all() {
+  macro_goto_code
+  // CHECK-MESSAGES-MACRO: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES-MACRO: [[@LINE-2]]:3: note: label defined here
+}
+
+void inside_macro_label() {
+  noop();
+  goto jump_to_me;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE+2]]:3: note: label defined here
+  noop();
+  macro_goto_label
+}
+
+void inside_macro_goto() {
+  noop();
+  macro_goto_jump
+  // CHECK-MESSAGES-MACRO: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES-MACRO: [[@LINE+2]]:3: note: label defined here
+  noop();
+  jump_to_me:;
+}

[HerrCai0907]

If set to true, the check will not warn if both label and goto statement are placed inside a macro. Default is true.

According to doc, it should be warn also since label is not in macro.

[vbvictor]

Thank you for the catch, my initial suggestion would be to ignore if both goto and label are in macro but later relaxed only to goto in macros.
What do you think will be better, behavior like in doc or like in actual tests?

[vbvictor]

Changed docs to match current behavior. We warn even if labels is not in macro because goto is the main "evil" here.

[carlosgalvezp]

LGTM!