[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE322626: [clang-tidy] implement check for goto (authored by 
JonasToth, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D41815?vs=130113&id=130115#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/hicpp/HICPPTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/hicpp-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
===
--- clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
+++ clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
@@ -0,0 +1,55 @@
+//===--- AvoidGotoCheck.cpp - clang-tidy---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "AvoidGotoCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+AST_MATCHER(GotoStmt, isForwardJumping) {
+  return Node.getLocStart() < Node.getLabel()->getLocStart();
+}
+
+void AvoidGotoCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+return;
+
+  // TODO: This check does not recognize `IndirectGotoStmt` which is a
+  // GNU extension. These must be matched separately and an AST matcher
+  // is currently missing for them.
+
+  // Check if the 'goto' is used for control flow other than jumping
+  // out of a nested loop.
+  auto Loop = stmt(anyOf(forStmt(), cxxForRangeStmt(), whileStmt(), doStmt()));
+  auto NestedLoop =
+  stmt(anyOf(forStmt(hasAncestor(Loop)), cxxForRangeStmt(hasAncestor(Loop)),
+ whileStmt(hasAncestor(Loop)), doStmt(hasAncestor(Loop;
+
+  Finder->addMatcher(gotoStmt(anyOf(unless(hasAncestor(NestedLoop)),
+unless(isForwardJumping(
+ .bind("goto"),
+ this);
+}
+
+void AvoidGotoCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Goto = Result.Nodes.getNodeAs("goto");
+
+  diag(Goto->getGotoLoc(), "avoid using 'goto' for flow control")
+  << Goto->getSourceRange();
+  diag(Goto->getLabel()->getLocStart(), "label defined here",
+   DiagnosticIDs::Note);
+}
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
===
--- clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
+++ clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
@@ -0,0 +1,36 @@
+//===--- AvoidGotoCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDGOTOCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDGOTOCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// The usage of ``goto`` for control flow is error prone and should be replaced
+/// with looping constructs. Only forward jumps in nested loops are accepted.
+//
+/// For the user-facing documentation see:
+/// 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) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDGOTOCHECK_H
Index: clang-tidy/cppcoreguidelines/CMakeLists.txt
===
--- clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -1,6 +1,7 @@
 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
+  AvoidGotoCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
   Interf

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 130113.
JonasToth marked an inline comment as done.
JonasToth added a comment.

- [Misc] remove spurious format


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/hicpp/HICPPTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/hicpp-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,139 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+void noop() {}
+
+int main() {
+  noop();
+  goto jump_to_me;
+  // 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:;
+
+jump_backwards:;
+  noop();
+  goto jump_backwards;
+  // 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-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:;
+  void *dynamic_label = &&some_label;
+
+  // FIXME: `IndirectGotoStmt` is not detected.
+  goto *dynamic_label;
+}
+
+void forward_jump_out_nested_loop() {
+  int array[] = {1, 2, 3, 4, 5};
+  for (int i = 0; i < 10; ++i) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (i + j > 10)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (int i = 0; i < 10; ++i) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (auto value : array) {
+noop();
+for (auto number : array) {
+  noop();
+  if (number == 5)
+goto early_exit1;
+}
+  }
+
+  do {
+noop();
+do {
+  noop();
+  goto early_exit1;
+} while (true);
+  } while (true);
+
+  do {
+for (auto number : array) {
+  noop();
+  if (number == 2)
+goto early_exit1;
+}
+  } while (true);
+
+  // Jumping further results in error, because the variable declaration would
+  // be skipped.
+early_exit1:;
+
+  int i = 0;
+  while (true) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit2;
+  i++;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (j > 5)
+goto early_exit2;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (auto number : array) {
+  if (number == 1)
+goto early_exit2;
+  noop();
+}
+  }
+
+  while (true) {
+noop();
+do {
+  noop();
+  goto early_exit2;
+} while (true);
+  }
+early_exit2:;
+}
+
+void jump_out_backwards() {
+
+before_the_loop:
+  noop();
+
+  for (int i = 0; i < 10; ++i) {
+for (int j = 0; j < 10; ++j) {
+  if (i * j > 80)
+goto before_the_loop;
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-8]]:1: note: label defined here
+}
+  }
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
@@ -90,6 +91,7 @@
google-runtime-member-string-references
google-runtime-operator
google-runtime-references
+   hicpp-avoid-goto (redirects to cppcoreguidelines-avoid-goto) 
hicpp-braces-around-statements (redirects to readability-braces-around-statements) 
hicpp-deprecated-headers (redirects to modernize-deprecated-headers) 
hicpp-exception-baseclass
Index: docs/clang-tidy/checks/hicpp-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/hicpp-avoid-goto.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - hicpp-avoid-goto
+
+hicpp-avoid-goto
+
+
+The `hicpp-avoid-goto` check is an alias to 
+`cppcoreguidelines-avoid-goto `_.
+Rule `6.3.1 High Integrity C++ 

[PATCH] D41815: [clang-tidy] implement check for goto

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from a minor formatting nit.




Comment at: clang-tidy/hicpp/HICPPTidyModule.cpp:57
 "hicpp-exception-baseclass");
-CheckFactories.registerCheck(
-"hicpp-signed-bitwise");
+CheckFactories.registerCheck("hicpp-signed-bitwise");
 CheckFactories.registerCheck(

Spurious formatting change -- feel free to commit separately (no review 
required) if you want to fix it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129901.
JonasToth added a comment.

- last nits i found


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/hicpp/HICPPTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/hicpp-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,139 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+void noop() {}
+
+int main() {
+  noop();
+  goto jump_to_me;
+  // 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:;
+
+jump_backwards:;
+  noop();
+  goto jump_backwards;
+  // 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-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:;
+  void *dynamic_label = &&some_label;
+
+  // FIXME: `IndirectGotoStmt` is not detected.
+  goto *dynamic_label;
+}
+
+void forward_jump_out_nested_loop() {
+  int array[] = {1, 2, 3, 4, 5};
+  for (int i = 0; i < 10; ++i) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (i + j > 10)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (int i = 0; i < 10; ++i) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (auto value : array) {
+noop();
+for (auto number : array) {
+  noop();
+  if (number == 5)
+goto early_exit1;
+}
+  }
+
+  do {
+noop();
+do {
+  noop();
+  goto early_exit1;
+} while (true);
+  } while (true);
+
+  do {
+for (auto number : array) {
+  noop();
+  if (number == 2)
+goto early_exit1;
+}
+  } while (true);
+
+  // Jumping further results in error, because the variable declaration would
+  // be skipped.
+early_exit1:;
+
+  int i = 0;
+  while (true) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit2;
+  i++;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (j > 5)
+goto early_exit2;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (auto number : array) {
+  if (number == 1)
+goto early_exit2;
+  noop();
+}
+  }
+
+  while (true) {
+noop();
+do {
+  noop();
+  goto early_exit2;
+} while (true);
+  }
+early_exit2:;
+}
+
+void jump_out_backwards() {
+
+before_the_loop:
+  noop();
+
+  for (int i = 0; i < 10; ++i) {
+for (int j = 0; j < 10; ++j) {
+  if (i * j > 80)
+goto before_the_loop;
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-8]]:1: note: label defined here
+}
+  }
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
@@ -90,6 +91,7 @@
google-runtime-member-string-references
google-runtime-operator
google-runtime-references
+   hicpp-avoid-goto (redirects to cppcoreguidelines-avoid-goto) 
hicpp-braces-around-statements (redirects to readability-braces-around-statements) 
hicpp-deprecated-headers (redirects to modernize-deprecated-headers) 
hicpp-exception-baseclass
Index: docs/clang-tidy/checks/hicpp-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/hicpp-avoid-goto.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - hicpp-avoid-goto
+
+hicpp-avoid-goto
+
+
+The `hicpp-avoid-goto` check is an alias to 
+`cppcoreguidelines-avoid-goto `_.
+Rule `6.3.1 High Integrity C++ 

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129900.
JonasToth added a comment.

- remove spurious whitespace


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/hicpp/HICPPTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/hicpp-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,139 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+void noop() {}
+
+int main() {
+  noop();
+  goto jump_to_me;
+  // 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:;
+
+jump_backwards:;
+  noop();
+  goto jump_backwards;
+  // 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-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:;
+  void *dynamic_label = &&some_label;
+
+  // FIXME: `IndirectGotoStmt` is not detected.
+  goto *dynamic_label;
+}
+
+void forward_jump_out_nested_loop() {
+  int array[] = {1, 2, 3, 4, 5};
+  for (int i = 0; i < 10; ++i) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (i + j > 10)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (int i = 0; i < 10; ++i) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (auto value : array) {
+noop();
+for (auto number : array) {
+  noop();
+  if (number == 5)
+goto early_exit1;
+}
+  }
+
+  do {
+noop();
+do {
+  noop();
+  goto early_exit1;
+} while (true);
+  } while (true);
+
+  do {
+for (auto number : array) {
+  noop();
+  if (number == 2)
+goto early_exit1;
+}
+  } while (true);
+
+  // Jumping further results in error, because the variable declaration would
+  // be skipped.
+early_exit1:;
+
+  int i = 0;
+  while (true) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit2;
+  i++;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (j > 5)
+goto early_exit2;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (auto number : array) {
+  if (number == 1)
+goto early_exit2;
+  noop();
+}
+  }
+
+  while (true) {
+noop();
+do {
+  noop();
+  goto early_exit2;
+} while (true);
+  }
+early_exit2:;
+}
+
+void jump_out_backwards() {
+
+before_the_loop:
+  noop();
+
+  for (int i = 0; i < 10; ++i) {
+for (int j = 0; j < 10; ++j) {
+  if (i * j > 80)
+goto before_the_loop;
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-8]]:1: note: label defined here
+}
+  }
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
@@ -90,6 +91,7 @@
google-runtime-member-string-references
google-runtime-operator
google-runtime-references
+   hicpp-avoid-goto (redirects to cppcoreguidelines-avoid-goto) 
hicpp-braces-around-statements (redirects to readability-braces-around-statements) 
hicpp-deprecated-headers (redirects to modernize-deprecated-headers) 
hicpp-exception-baseclass
Index: docs/clang-tidy/checks/hicpp-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/hicpp-avoid-goto.rst
@@ -0,0 +1,11 @@
+.. title:: clang-tidy - hicpp-avoid-goto
+
+hicpp-avoid-goto
+
+
+The `hicpp-avoid-go` check is an alias to 
+`cppcoreguidelines-avoid-goto `_.
+Rule `6.3.1 High Integrity C++ 

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129899.
JonasToth marked 3 inline comments as done.
JonasToth added a comment.

- hicpp alias added
- update documentation


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/hicpp/HICPPTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/hicpp-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,139 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+void noop() {}
+
+int main() {
+  noop();
+  goto jump_to_me;
+  // 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:;
+
+jump_backwards:;
+  noop();
+  goto jump_backwards;
+  // 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-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:;
+  void *dynamic_label = &&some_label;
+
+  // FIXME: `IndirectGotoStmt` is not detected.
+  goto *dynamic_label;
+}
+
+void forward_jump_out_nested_loop() {
+  int array[] = {1, 2, 3, 4, 5};
+  for (int i = 0; i < 10; ++i) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (i + j > 10)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (int i = 0; i < 10; ++i) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (auto value : array) {
+noop();
+for (auto number : array) {
+  noop();
+  if (number == 5)
+goto early_exit1;
+}
+  }
+
+  do {
+noop();
+do {
+  noop();
+  goto early_exit1;
+} while (true);
+  } while (true);
+
+  do {
+for (auto number : array) {
+  noop();
+  if (number == 2)
+goto early_exit1;
+}
+  } while (true);
+
+  // Jumping further results in error, because the variable declaration would
+  // be skipped.
+early_exit1:;
+
+  int i = 0;
+  while (true) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit2;
+  i++;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (j > 5)
+goto early_exit2;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (auto number : array) {
+  if (number == 1)
+goto early_exit2;
+  noop();
+}
+  }
+
+  while (true) {
+noop();
+do {
+  noop();
+  goto early_exit2;
+} while (true);
+  }
+early_exit2:;
+}
+
+void jump_out_backwards() {
+
+before_the_loop:
+  noop();
+
+  for (int i = 0; i < 10; ++i) {
+for (int j = 0; j < 10; ++j) {
+  if (i * j > 80)
+goto before_the_loop;
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-8]]:1: note: label defined here
+}
+  }
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
@@ -90,6 +91,7 @@
google-runtime-member-string-references
google-runtime-operator
google-runtime-references
+   hicpp-avoid-goto (redirects to cppcoreguidelines-avoid-goto) 
hicpp-braces-around-statements (redirects to readability-braces-around-statements) 
hicpp-deprecated-headers (redirects to modernize-deprecated-headers) 
hicpp-exception-baseclass
Index: docs/clang-tidy/checks/hicpp-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/hicpp-avoid-goto.rst
@@ -0,0 +1,11 @@
+.. title:: clang-tidy - hicpp-avoid-goto
+
+hicpp-avoid-goto
+
+
+The `hicpp-avoid-go` check is an alias to 
+`cppcoreguidelines-avoid-goto `_.
+Rule `6.3.1 High Integrity C++ 

[PATCH] D41815: [clang-tidy] implement check for goto

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.h:19-20
+
+/// The usage of `goto` has been discouraged for a long time and is diagnosed
+/// with this check.
+///

This comment is a bit out of date now that it no longer diagnoses all uses of 
`goto`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129868.
JonasToth added a comment.

- doc clarified that check is c++ only
- add missing tests for `do` and `range-for`, not all combinations done, but 
every loop construct is used as outer and inner loop


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,139 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+void noop() {}
+
+int main() {
+  noop();
+  goto jump_to_me;
+  // 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:;
+
+jump_backwards:;
+  noop();
+  goto jump_backwards;
+  // 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-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:;
+  void *dynamic_label = &&some_label;
+
+  // FIXME: `IndirectGotoStmt` is not detected.
+  goto *dynamic_label;
+}
+
+void forward_jump_out_nested_loop() {
+  int array[] = {1, 2, 3, 4, 5};
+  for (int i = 0; i < 10; ++i) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (i + j > 10)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (int i = 0; i < 10; ++i) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (auto value : array) {
+noop();
+for (auto number : array) {
+  noop();
+  if (number == 5)
+goto early_exit1;
+}
+  }
+
+  do {
+noop();
+do {
+  noop();
+  goto early_exit1;
+} while (true);
+  } while (true);
+
+  do {
+for (auto number : array) {
+  noop();
+  if (number == 2)
+goto early_exit1;
+}
+  } while (true);
+
+  // Jumping further results in error, because the variable declaration would
+  // be skipped.
+early_exit1:;
+
+  int i = 0;
+  while (true) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit2;
+  i++;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (j > 5)
+goto early_exit2;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (auto number : array) {
+  if (number == 1)
+goto early_exit2;
+  noop();
+}
+  }
+
+  while (true) {
+noop();
+do {
+  noop();
+  goto early_exit2;
+} while (true);
+  }
+early_exit2:;
+}
+
+void jump_out_backwards() {
+
+before_the_loop:
+  noop();
+
+  for (int i = 0; i < 10; ++i) {
+for (int j = 0; j < 10; ++j) {
+  if (i * j > 80)
+goto before_the_loop;
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-8]]:1: note: label defined here
+}
+  }
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
Index: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
@@ -0,0 +1,50 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-goto
+
+cppcoreguidelines-avoid-goto
+
+
+The usage of ``goto`` for control flow is error prone and should be replaced
+with looping constructs. Only forward jumps in nested loops are accepted.
+
+This check implements `ES.76 `_ 
+from the CppCoreGuidelines and 
+`6.3.1 from High Integrity C++ 

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129865.
JonasToth marked an inline comment as done.
JonasToth added a comment.

- [Fix] bug with language mode


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,94 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+void noop() {}
+
+int main() {
+  noop();
+  goto jump_to_me;
+  // 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:;
+
+jump_backwards:;
+  noop();
+  goto jump_backwards;
+  // 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-MESSAGES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-2]]:24: note: label defined here
+
+  // Test the GNU extension https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html
+some_label:;
+  void *dynamic_label = &&some_label;
+
+  // FIXME: Not detected
+  goto *dynamic_label;
+}
+
+void forward_jump_out_nested_loop() {
+  for (int i = 0; i < 10; ++i) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (i + j > 10)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (int i = 0; i < 10; ++i) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit1;
+}
+noop();
+  }
+  // Jumping further results in error, because the variable declaration would
+  // be skipped.
+early_exit1:;
+
+  int i = 0;
+  while (true) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit2;
+  i++;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (j > 5)
+goto early_exit2;
+}
+noop();
+  }
+
+early_exit2:;
+}
+
+void jump_out_backwards() {
+
+before_the_loop:
+  noop();
+
+  for (int i = 0; i < 10; ++i) {
+for (int j = 0; j < 10; ++j) {
+  if (i * j > 80)
+goto before_the_loop;
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-8]]:1: note: label defined here
+}
+  }
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
Index: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
@@ -0,0 +1,50 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-goto
+
+cppcoreguidelines-avoid-goto
+
+
+The usage of ``goto`` for control flow is error prone and should be replaced
+with looping constructs. Only forward jumps in nested loops are accepted.
+
+This check implements `ES.76 `_ 
+from the CppCoreGuidelines and 
+`6.3.1 from High Integrity C++ `_.
+
+For more information on why to avoid programming 
+with ``goto`` you can read the famous paper `A Case against the GO TO Statement. `_.
+
+The check diagnoses ``goto`` for backward jumps in every language mode. These
+should be replaced with `C/C++` looping constructs.
+
+.. code-block:: c++
+
+  // Bad, handwritten for loop.
+  int i = 0;
+  // Jump label for the loop
+  loop_start:
+  do_some_operation();
+
+  if (i < 100) {
+++i;
+goto loop_start;
+  }
+
+  // Better
+  for(int i = 0; i < 100; ++i)
+do_some_operation();
+
+Modern C++ needs ``goto`` only to jump out of nested loops.
+
+.. code-block:: c++
+
+  for(int i = 0; i < 100; ++i) {
+for(int j = 0

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129863.
JonasToth marked 4 inline comments as done.
JonasToth added a comment.

- address review comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,94 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+void noop() {}
+
+int main() {
+  noop();
+  goto jump_to_me;
+  // 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:;
+
+jump_backwards:;
+  noop();
+  goto jump_backwards;
+  // 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-MESSAGES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-2]]:24: note: label defined here
+
+  // Test the GNU extension https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html
+some_label:;
+  void *dynamic_label = &&some_label;
+
+  // FIXME: Not detected
+  goto *dynamic_label;
+}
+
+void forward_jump_out_nested_loop() {
+  for (int i = 0; i < 10; ++i) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (i + j > 10)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (int i = 0; i < 10; ++i) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit1;
+}
+noop();
+  }
+  // Jumping further results in error, because the variable declaration would
+  // be skipped.
+early_exit1:;
+
+  int i = 0;
+  while (true) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit2;
+  i++;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (j > 5)
+goto early_exit2;
+}
+noop();
+  }
+
+early_exit2:;
+}
+
+void jump_out_backwards() {
+
+before_the_loop:
+  noop();
+
+  for (int i = 0; i < 10; ++i) {
+for (int j = 0; j < 10; ++j) {
+  if (i * j > 80)
+goto before_the_loop;
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-8]]:1: note: label defined here
+}
+  }
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
Index: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
@@ -0,0 +1,50 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-goto
+
+cppcoreguidelines-avoid-goto
+
+
+The usage of ``goto`` for control flow is error prone and should be replaced
+with looping constructs. Only forward jumps in nested loops are accepted.
+
+This check implements `ES.76 `_ 
+from the CppCoreGuidelines and 
+`6.3.1 from High Integrity C++ `_.
+
+For more information on why to avoid programming 
+with ``goto`` you can read the famous paper `A Case against the GO TO Statement. `_.
+
+The check diagnoses ``goto`` for backward jumps in every language mode. These
+should be replaced with `C/C++` looping constructs.
+
+.. code-block:: c++
+
+  // Bad, handwritten for loop.
+  int i = 0;
+  // Jump label for the loop
+  loop_start:
+  do_some_operation();
+
+  if (i < 100) {
+++i;
+goto loop_start;
+  }
+
+  // Better
+  for(int i = 0; i < 100; ++i)
+do_some_operation();
+
+Modern C++ needs ``goto`` only to jump out of nested loops.
+
+.. code-block:: c++
+
+  for(int i = 0; i < 100; ++i) {
+for(int j = 0; j <

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:43
+
+  // Backward jumps are diagnosed in all language modes. Forward jumps
+  // are sometimes required in C to free resources or do other clean-up

aaron.ballman wrote:
> I think that this check should be C++ only. C makes far more use of `goto`, 
> and backwards jumps are not always bad there (they don't have to consider 
> things like destructors or RAII like you do in C++).
> 
> Esp since this is a check for the C++ core guidelines and HICPP (both are C++ 
> standards). 
Ok. I merged the matchers too.



Comment at: docs/ReleaseNotes.rst:67-68
 
+- New `cppcoreguidelines-avoid-goto
+  
`_
 check
+

aaron.ballman wrote:
> I think you should also add the HICPP changes as well, given that this check 
> also covers that rule.
I think `Only forward jumps in nested loops are accepted.` covers it, but i 
reformulated it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 3 inline comments as done.
JonasToth added inline comments.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:21
+AST_MATCHER(GotoStmt, isForwardJumping) {
+  return Node.getLocStart() < Node.getLabel()->getLocStart();
+}

lebedev.ri wrote:
> JonasToth wrote:
> > lebedev.ri wrote:
> > > Hm, on a second thought, i think this will have false-positive if the 
> > > label and the goto are on the same line, like
> > > ```
> > > goto label; ; label: ; 
> > > ```
> > > I wonder we could **easily** compare accounting for the position in the 
> > > line, or it is not worth the extra complexity.
> > Iam not sure what you mean. Is the concern that the `goto` does not 
> > actually skip something?
> > If yes it is still worth it, because its not necessary to have the `goto` 
> > then.
> > 
> > The `<` on the locations should always be sensefull because AFAIK its at 
> > byte level (or something similar).
> > 
> > Your example is diagnosed correctly.
> > The < on the locations should always be sensefull because AFAIK its at byte 
> > level (or something similar).
> Ah, great then. I had some recollections of having some issues with that 
> before, but it makes sense that it would just work.
> 
I dont know the internals, i just assume because it works :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:20
+
+AST_MATCHER(GotoStmt, isForwardJumping) {
+

JonasToth wrote:
> lebedev.ri wrote:
> > JonasToth wrote:
> > > lebedev.ri wrote:
> > > > It would be nice to have it in standard ASTMatchers, i believe it will 
> > > > be useful for `else-after-return` check.
> > > > Though the ASTMatchers are stuck due to the doc-dumping script being 
> > > > 'broken' (see D41455)
> > > Yes. The GNU extension goto does not have ASTMatcher yet, so i will pack 
> > > these together in a review. What do you think how long the ASTMatcher 
> > > issue will be there? Maybe it could be done after that check lands?
> > > Maybe it could be done after that check lands?
> > 
> > Yes, absolutely. I did not meant that as a blocker here.
> On my todo list.
I'm not certain adding this to the AST matchers is critical. For instance, we 
may want to, instead, consider adding relational operators for source locations 
and then expose a generic facility for getting source location information out 
of AST nodes.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:26
+  // TODO: This check does not recognize `IndirectGotoStmt` which is a
+  // GNU extension. These must be matched separatly and a ASTMatcher
+  // is currently missing for them. Adding this matcher and moving

separatly -> separately
a ASTMatcher -> an AST matcher



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:28-29
+  // is currently missing for them. Adding this matcher and moving
+  // `isForwardJumping` (as requested by LebedevRI) to ASTMatchers
+  // will be done together.
+

We don't usually put names into the source code and given the questions about 
adding that matcher, I'd say it's better to remove the promise that this will 
become a generic one.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:43
+
+  // Backward jumps are diagnosed in all language modes. Forward jumps
+  // are sometimes required in C to free resources or do other clean-up

I think that this check should be C++ only. C makes far more use of `goto`, and 
backwards jumps are not always bad there (they don't have to consider things 
like destructors or RAII like you do in C++).

Esp since this is a check for the C++ core guidelines and HICPP (both are C++ 
standards). 



Comment at: docs/ReleaseNotes.rst:67-68
 
+- New `cppcoreguidelines-avoid-goto
+  
`_
 check
+

I think you should also add the HICPP changes as well, given that this check 
also covers that rule.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:21
+AST_MATCHER(GotoStmt, isForwardJumping) {
+  return Node.getLocStart() < Node.getLabel()->getLocStart();
+}

JonasToth wrote:
> lebedev.ri wrote:
> > Hm, on a second thought, i think this will have false-positive if the label 
> > and the goto are on the same line, like
> > ```
> > goto label; ; label: ; 
> > ```
> > I wonder we could **easily** compare accounting for the position in the 
> > line, or it is not worth the extra complexity.
> Iam not sure what you mean. Is the concern that the `goto` does not actually 
> skip something?
> If yes it is still worth it, because its not necessary to have the `goto` 
> then.
> 
> The `<` on the locations should always be sensefull because AFAIK its at byte 
> level (or something similar).
> 
> Your example is diagnosed correctly.
> The < on the locations should always be sensefull because AFAIK its at byte 
> level (or something similar).
Ah, great then. I had some recollections of having some issues with that 
before, but it makes sense that it would just work.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:21
+AST_MATCHER(GotoStmt, isForwardJumping) {
+  return Node.getLocStart() < Node.getLabel()->getLocStart();
+}

lebedev.ri wrote:
> Hm, on a second thought, i think this will have false-positive if the label 
> and the goto are on the same line, like
> ```
> goto label; ; label: ; 
> ```
> I wonder we could **easily** compare accounting for the position in the line, 
> or it is not worth the extra complexity.
Iam not sure what you mean. Is the concern that the `goto` does not actually 
skip something?
If yes it is still worth it, because its not necessary to have the `goto` then.

The `<` on the locations should always be sensefull because AFAIK its at byte 
level (or something similar).

Your example is diagnosed correctly.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129760.
JonasToth added a comment.

- add edgecase test from roman


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,94 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+void noop() {}
+
+int main() {
+  noop();
+  goto jump_to_me;
+  // 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:;
+
+jump_backwards:;
+  noop();
+  goto jump_backwards;
+  // 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-MESSAGES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-2]]:24: note: label defined here
+
+  // Test the GNU extension https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html
+some_label:;
+  void *dynamic_label = &&some_label;
+
+  // FIXME: Not detected
+  goto *dynamic_label;
+}
+
+void forward_jump_out_nested_loop() {
+  for (int i = 0; i < 10; ++i) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (i + j > 10)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (int i = 0; i < 10; ++i) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit1;
+}
+noop();
+  }
+  // Jumping further results in error, because the variable declaration would
+  // be skipped.
+early_exit1:;
+
+  int i = 0;
+  while (true) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit2;
+  i++;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (j > 5)
+goto early_exit2;
+}
+noop();
+  }
+
+early_exit2:;
+}
+
+void jump_out_backwards() {
+
+before_the_loop:
+  noop();
+
+  for (int i = 0; i < 10; ++i) {
+for (int j = 0; j < 10; ++j) {
+  if (i * j > 80)
+goto before_the_loop;
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-8]]:1: note: label defined here
+}
+  }
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
Index: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
@@ -0,0 +1,50 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-goto
+
+cppcoreguidelines-avoid-goto
+
+
+The usage of ``goto`` for control flow is error prone and should be replaced
+with looping constructs. Only forward jumps in nested loops are accepted.
+
+This check implements `ES.76 `_ 
+from the CppCoreGuidelines and 
+`6.3.1 from High Integrity C++ `_.
+
+For more information on why to avoid programming 
+with ``goto`` you can read the famous paper `A Case against the GO TO Statement. `_.
+
+The check diagnoses ``goto`` for backward jumps in every language mode. These
+should be replaced with `C/C++` looping constructs.
+
+.. code-block:: c++
+
+  // Bad, handwritten for loop.
+  int i = 0;
+  // Jump label for the loop
+  loop_start:
+  do_some_operation();
+
+  if (i < 100) {
+++i;
+goto loop_start;
+  }
+
+  // Better
+  for(int i = 0; i < 100; ++i)
+do_some_operation();
+
+Modern C++ needs ``goto`` only to jump out of nested loops.
+
+.. code-block:: c++
+
+  for(int i = 0; i < 100; ++i) {
+for(int j = 0; j < 100; ++j) {
+  if (i * j > 500)
+ 

[PATCH] D41815: [clang-tidy] implement check for goto

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:21
+AST_MATCHER(GotoStmt, isForwardJumping) {
+  return Node.getLocStart() < Node.getLabel()->getLocStart();
+}

Hm, on a second thought, i think this will have false-positive if the label and 
the goto are on the same line, like
```
goto label; ; label: ; 
```
I wonder we could **easily** compare accounting for the position in the line, 
or it is not worth the extra complexity.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst:6-7
+
+The usage of ``goto`` has been discouraged for a long time and is diagnosed
+with this check.
+

JonasToth wrote:
> aaron.ballman wrote:
> > This doesn't really help the user understand what's bad about goto or why 
> > it should be diagnosed. You should expound a bit here.
> Is it better now?
Much better now! Thank you for fix!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129684.
JonasToth added a comment.

- simplified the code and merged diagnostics


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,89 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+void noop() {}
+
+int main() {
+  noop();
+  goto jump_to_me;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+  noop();
+
+jump_to_me:;
+
+jump_backwards:;
+  noop();
+  goto jump_backwards;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-4]]:1: note: label defined here
+
+  // Test the GNU extension https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html
+some_label:;
+  void *dynamic_label = &&some_label;
+
+  // FIXME: Not detected
+  goto *dynamic_label;
+}
+
+void forward_jump_out_nested_loop() {
+  for (int i = 0; i < 10; ++i) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (i + j > 10)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (int i = 0; i < 10; ++i) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit1;
+}
+noop();
+  }
+  // Jumping further results in error, because the variable declaration would
+  // be skipped.
+early_exit1:;
+
+  int i = 0;
+  while (true) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit2;
+  i++;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (j > 5)
+goto early_exit2;
+}
+noop();
+  }
+
+early_exit2:;
+}
+
+void jump_out_backwards() {
+
+before_the_loop:
+  noop();
+
+  for (int i = 0; i < 10; ++i) {
+for (int j = 0; j < 10; ++j) {
+  if (i * j > 80)
+goto before_the_loop;
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-8]]:1: note: label defined here
+}
+  }
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
Index: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
@@ -0,0 +1,50 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-goto
+
+cppcoreguidelines-avoid-goto
+
+
+The usage of ``goto`` for control flow is error prone and should be replaced
+with looping constructs. Only forward jumps in nested loops are accepted.
+
+This check implements `ES.76 `_ 
+from the CppCoreGuidelines and 
+`6.3.1 from High Integrity C++ `_.
+
+For more information on why to avoid programming 
+with ``goto`` you can read the famous paper `A Case against the GO TO Statement. `_.
+
+The check diagnoses ``goto`` for backward jumps in every language mode. These
+should be replaced with `C/C++` looping constructs.
+
+.. code-block:: c++
+
+  // Bad, handwritten for loop.
+  int i = 0;
+  // Jump label for the loop
+  loop_start:
+  do_some_operation();
+
+  if (i < 100) {
+++i;
+goto loop_start;
+  }
+
+  // Better
+  for(int i = 0; i < 100; ++i)
+do_some_operation();
+
+Modern C++ needs ``goto`` only to jump out of nested loops.
+
+.. code-block:: c++
+
+  for(int i = 0; i < 100; ++i) {
+for(int j = 0; j < 100; ++j) {
+  if (i * j > 500)
+goto early_exit;
+}
+  }
+
+  early_exit:
+  some_operation();
+
+For C++11 or higher all other uses of ``goto`` are diagnosed.
Index: docs/ReleaseNotes.rst
==

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 5 inline comments as done.
JonasToth added inline comments.



Comment at: docs/ReleaseNotes.rst:63
+
+  The usage of ``goto`` has been discouraged for a long time and is diagnosed
+  with this check.

Eugene.Zelenko wrote:
> I think will be good idea to reformulate this statement and its copy in 
> documentation. //diagnosed with this check// is tautological for any check.
Reformulated, is it ok now?



Comment at: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst:6-7
+
+The usage of ``goto`` has been discouraged for a long time and is diagnosed
+with this check.
+

aaron.ballman wrote:
> This doesn't really help the user understand what's bad about goto or why it 
> should be diagnosed. You should expound a bit here.
Is it better now?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129677.
JonasToth marked 8 inline comments as done.
JonasToth added a comment.

- address review comments
- add better documentation with code examples


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,91 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+void noop() {}
+
+int main() {
+  noop();
+  goto jump_to_me;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+  noop();
+
+jump_to_me:;
+
+jump_backwards:;
+  noop();
+  goto jump_backwards;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-2]]:3: warning: do not jump backwards with 'goto'
+  // CHECK-MESSAGES: [[@LINE-5]]:1: note: label defined here
+
+  // Test the GNU extension https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html
+some_label:;
+  void *dynamic_label = &&some_label;
+
+  // FIXME: Not detected
+  goto *dynamic_label;
+  // CHECK MESSAGES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+}
+
+void forward_jump_out_nested_loop() {
+  for (int i = 0; i < 10; ++i) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (i + j > 10)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (int i = 0; i < 10; ++i) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit1;
+}
+noop();
+  }
+  // Jumping further results in error, because the variable declaration would
+  // be skipped.
+early_exit1:;
+
+  int i = 0;
+  while (true) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit2;
+  i++;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (j > 5)
+goto early_exit2;
+}
+noop();
+  }
+
+early_exit2:;
+}
+
+void jump_out_backwards() {
+
+before_the_loop:
+  noop();
+
+  for (int i = 0; i < 10; ++i) {
+for (int j = 0; j < 10; ++j) {
+  if (i * j > 80)
+goto before_the_loop;
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: do not jump backwards with 'goto'
+  // CHECK-MESSAGES: [[@LINE-8]]:1: note: label defined here
+}
+  }
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
Index: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
@@ -0,0 +1,50 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-goto
+
+cppcoreguidelines-avoid-goto
+
+
+The usage of ``goto`` for control flow is error prone and should be replaced
+with looping constructs. Only forward jumps in nested loops are accepted.
+
+This check implements `ES.76 `_ 
+from the CppCoreGuidelines and 
+`6.3.1 from High Integrity C++ `_.
+
+For more information on why to avoid programming 
+with ``goto`` you can read the famous paper `A Case against the GO TO Statement. `_.
+
+The check diagnoses ``goto`` for backward jumps in every language mode. These
+should be replaced with `C/C++` looping constructs.
+
+.. code-block:: c++
+
+  // Bad, handwritten for loop.
+  int i = 0;
+  // Jump label for the loop
+  loop_start:
+  do_some_operation();
+
+  if (i < 100) {
+++i;
+goto loop_start;
+  }
+
+  // Better
+  for(int i = 0; i < 100; ++i)
+do_some_operation();
+
+Modern C++ needs ``goto`` only to jump out of nested loops.
+
+.. code-block:: c++
+
+  for(int i = 0; i < 100; ++i) {
+for(int j = 0; j < 100; ++j) {
+  if (i * j > 500)
+

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 3 inline comments as done.
JonasToth added inline comments.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:20
+
+AST_MATCHER(GotoStmt, isForwardJumping) {
+

lebedev.ri wrote:
> JonasToth wrote:
> > lebedev.ri wrote:
> > > It would be nice to have it in standard ASTMatchers, i believe it will be 
> > > useful for `else-after-return` check.
> > > Though the ASTMatchers are stuck due to the doc-dumping script being 
> > > 'broken' (see D41455)
> > Yes. The GNU extension goto does not have ASTMatcher yet, so i will pack 
> > these together in a review. What do you think how long the ASTMatcher issue 
> > will be there? Maybe it could be done after that check lands?
> > Maybe it could be done after that check lands?
> 
> Yes, absolutely. I did not meant that as a blocker here.
On my todo list.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:20
+
+AST_MATCHER(GotoStmt, isForwardJumping) {
+

lebedev.ri wrote:
> It would be nice to have it in standard ASTMatchers, i believe it will be 
> useful for `else-after-return` check.
> Though the ASTMatchers are stuck due to the doc-dumping script being 'broken' 
> (see D41455)
Yes. The GNU extension goto does not have ASTMatcher yet, so i will pack these 
together in a review. What do you think how long the ASTMatcher issue will be 
there? Maybe it could be done after that check lands?



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:41-53
+  if (const auto *BackGoto =
+  Result.Nodes.getNodeAs("backward-goto")) {
+diag(BackGoto->getGotoLoc(), "do not jump backwards with 'goto'")
+<< BackGoto->getSourceRange();
+diag(BackGoto->getLabel()->getLocStart(), "label defined here",
+ DiagnosticIDs::Note);
+  }

aaron.ballman wrote:
> Is there a reason to have two separate diagnostics? It seems like these both 
> would be covered by "this use of 'goto' for flow control is prohibited".
Yes, the new wording better. The note about the backward jump could be added in 
the `label defined here` note. I think distignuishing between backward jumps 
and forward jumps is still a good thing.

The forward jumps could come from a C code part where forward jumps are done 
for resource cleaning. So having a strong `prohibited` and a suggesting `avoid` 
diagnostic makes sense to me.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:20
+
+AST_MATCHER(GotoStmt, isForwardJumping) {
+

JonasToth wrote:
> lebedev.ri wrote:
> > It would be nice to have it in standard ASTMatchers, i believe it will be 
> > useful for `else-after-return` check.
> > Though the ASTMatchers are stuck due to the doc-dumping script being 
> > 'broken' (see D41455)
> Yes. The GNU extension goto does not have ASTMatcher yet, so i will pack 
> these together in a review. What do you think how long the ASTMatcher issue 
> will be there? Maybe it could be done after that check lands?
> Maybe it could be done after that check lands?

Yes, absolutely. I did not meant that as a blocker here.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D41815#973265, @lebedev.ri wrote:

> In https://reviews.llvm.org/D41815#973260, @JonasToth wrote:
>
> > - check that jumps will only be forward. AFAIK that is required in all 
> > sensefull usecases of goto, is it?
>
>
> You could implement loops/recursion with goto, something like:
>
>   const int end = 10;
>   int i = 0;
>   assert(i < end);
>  
>   begin:
>   
>   i++
>   if(i < end)
> goto begin;
>  
>   // end
>
>
> But it really should be done with normal `for()`, or `while()`, so i think it 
> would make sense to diagnose those.


That check is specifically targeting these code constellations to be bad 
practice. As you said, using the looping constructs is the way to go :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:20
+
+AST_MATCHER(GotoStmt, isForwardJumping) {
+

It would be nice to have it in standard ASTMatchers, i believe it will be 
useful for `else-after-return` check.
Though the ASTMatchers are stuck due to the doc-dumping script being 'broken' 
(see D41455)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D41815#973260, @JonasToth wrote:

> I enhanced the check to do more:
>
> - check that jumps will only be forward. AFAIK that is required in all 
> sensefull usecases of goto, is it?
> - additionally check for gotos in nested loops. These are not diagnosed if 
> the jump is forward implementing the exception in the core guidelines.
>
>   With these modifications the check can be used to enforce the rule in the 
> CoreGuidelines and the goto part of `6.3.1 Ensure that the label(s) for a 
> jump statement or a switch condition appear later, in the same or an 
> enclosing block` for the HICPP module.


Nice!

> Some test cases for all combinations are missing, i can add those once you 
>  agree that the functionality change is indeed ok.

I think this is the correct direction for the check.




Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:22
+  if (getLangOpts().CPlusPlus)
+Finder->addMatcher(gotoStmt().bind("goto"), this);
+}

JonasToth wrote:
> aaron.ballman wrote:
> > JonasToth wrote:
> > > aaron.ballman wrote:
> > > > aaron.ballman wrote:
> > > > > Are you planning to add the exception listed in the C++ Core 
> > > > > Guideline? It makes an explicit exception allowing you to jump 
> > > > > forward out of a loop construct.
> > > > What should this check do with indirect goto statements (it's a GCC 
> > > > extension we support where you can jump to an expression)?
> > > > 
> > > > Regardless, there should be a test for indirect gotos and jump forward 
> > > > out of loop constructs.
> > > > Are you planning to add the exception listed in the C++ Core Guideline? 
> > > > It makes an explicit exception allowing you to jump forward out of a 
> > > > loop construct.
> > > 
> > > I do plan for this. Because I dont have any experience with gotos I 
> > > wanted to do it in small steps.
> > > 
> > > > What should this check do with indirect goto statements (it's a GCC 
> > > > extension we support where you can jump to an expression)?
> > > Iam not aware of these :) 
> > >> What should this check do with indirect goto statements (it's a GCC 
> > >> extension we support where you can jump to an expression)?
> > >
> > > Iam not aware of these :)
> > 
> > https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html
> > (and a good reference on why these are interesting: 
> > https://eli.thegreenplace.net/2012/07/12/computed-goto-for-efficient-dispatch-tables)
> I would think that this is a special feature that will only be used if you 
> know what you are doing. So it should be allowed with configuration. I am not 
> sure about the default though. For now it is ignored.
> 
> HICPP has a rule on gotos as well, which states that only forward jumps are 
> allowed.
> 
> I think that these different approaches to `goto` should land here sometime 
> as different configurations.
> I would think that this is a special feature that will only be used if you 
> know what you are doing. So it should be allowed with configuration. I am not 
> sure about the default though. For now it is ignored.

Ignoring it for now seems reasonable to me. You should add a TODO comment for 
it so we don't lose track of it, though.

> HICPP has a rule on gotos as well, which states that only forward jumps are 
> allowed.

That's essentially the same exception as the C++ Core Guidelines, which is nice.

> I think that these different approaches to goto should land here sometime as 
> different configurations.

Agreed, if needed.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:21
+AST_MATCHER(GotoStmt, isForwardJumping) {
+
+  return Node.getLocStart() < Node.getLabel()->getLocStart();

Can remove the spurious newline.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:26
+void AvoidGotoCheck::registerMatchers(MatchFinder *Finder) {
+  // Check if the 'goto' is used for control flow other then jumping
+  // out of a nested loop.

s/then/than



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:41-53
+  if (const auto *BackGoto =
+  Result.Nodes.getNodeAs("backward-goto")) {
+diag(BackGoto->getGotoLoc(), "do not jump backwards with 'goto'")
+<< BackGoto->getSourceRange();
+diag(BackGoto->getLabel()->getLocStart(), "label defined here",
+ DiagnosticIDs::Note);
+  }

Is there a reason to have two separate diagnostics? It seems like these both 
would be covered by "this use of 'goto' for flow control is prohibited".


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D41815#973260, @JonasToth wrote:

> - check that jumps will only be forward. AFAIK that is required in all 
> sensefull usecases of goto, is it?


You could implement loops/recursion with goto, something like:

  const int end = 10;
  int i = 0;
  assert(i < end);
  
  begin:
  
  i++
  if(i < end)
goto begin;
  
  // end

But it really should be done with normal `for()`, or `while()`, so i think it 
would make sense to diagnose those.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129433.
JonasToth added a comment.

I enhanced the check to do more:

- check that jumps will only be forward. AFAIK that is required in all 
sensefull usecases of goto, is it?
- additionally check for gotos in nested loops. These are not diagnosed if the 
jump is forward implementing the exception in the core guidelines.

With these modifications the check can be used to enforce the rule in the 
CoreGuidelines and the goto part of `6.3.1 Ensure that the label(s) for a jump 
statement or a switch condition appear later, in the same or an enclosing block`
for the HICPP module.

Some test cases for all combinations are missing, i can add those once you 
agree that the functionality change is indeed ok.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,91 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+void noop() {}
+
+int main() {
+  noop();
+  goto jump_to_me;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+  noop();
+
+jump_to_me:;
+
+jump_backwards:;
+  noop();
+  goto jump_backwards;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+  // CHECK-MESSAGES: [[@LINE-2]]:3: warning: do not jump backwards with 'goto'
+  // CHECK-MESSAGES: [[@LINE-5]]:1: note: label defined here
+
+  // Test the GNU extension https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html
+some_label:;
+  void *dynamic_label = &&some_label;
+
+  // FIXME: Not detected
+  goto *dynamic_label;
+  // CHECK MESSAGES: [[@LINE-1]]:3: warning: avoid using 'goto' for flow control
+}
+
+void forward_jump_out_nested_loop() {
+  for (int i = 0; i < 10; ++i) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (i + j > 10)
+goto early_exit1;
+}
+noop();
+  }
+
+  for (int i = 0; i < 10; ++i) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit1;
+}
+noop();
+  }
+  // Jumping further results in error, because the variable declaration would
+  // be skipped.
+early_exit1:;
+
+  int i = 0;
+  while (true) {
+noop();
+while (true) {
+  noop();
+  if (i > 5)
+goto early_exit2;
+  i++;
+}
+noop();
+  }
+
+  while (true) {
+noop();
+for (int j = 0; j < 10; ++j) {
+  noop();
+  if (j > 5)
+goto early_exit2;
+}
+noop();
+  }
+
+early_exit2:;
+}
+
+void jump_out_backwards() {
+
+before_the_loop:
+  noop();
+
+  for (int i = 0; i < 10; ++i) {
+for (int j = 0; j < 10; ++j) {
+  if (i * j > 80)
+goto before_the_loop;
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: do not jump backwards with 'goto'
+  // CHECK-MESSAGES: [[@LINE-8]]:1: note: label defined here
+}
+  }
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
Index: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
@@ -0,0 +1,11 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-goto
+
+cppcoreguidelines-avoid-goto
+
+
+The usage of ``goto`` has been discouraged for a long time and is diagnosed
+with this check.
+
+This check implements `ES.76 `_ 
+from the CppCoreGuidelines. For more information on why to avoid programming 
+with ``goto`` you can read the famous paper `A Case against the GO TO Statement. `_.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
 Improvements to clang-tidy
 --
 
+- New `cppcoreguidelines-avoid-goto
+  

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:22
+  if (getLangOpts().CPlusPlus)
+Finder->addMatcher(gotoStmt().bind("goto"), this);
+}

aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > Are you planning to add the exception listed in the C++ Core Guideline? 
> > > > It makes an explicit exception allowing you to jump forward out of a 
> > > > loop construct.
> > > What should this check do with indirect goto statements (it's a GCC 
> > > extension we support where you can jump to an expression)?
> > > 
> > > Regardless, there should be a test for indirect gotos and jump forward 
> > > out of loop constructs.
> > > Are you planning to add the exception listed in the C++ Core Guideline? 
> > > It makes an explicit exception allowing you to jump forward out of a loop 
> > > construct.
> > 
> > I do plan for this. Because I dont have any experience with gotos I wanted 
> > to do it in small steps.
> > 
> > > What should this check do with indirect goto statements (it's a GCC 
> > > extension we support where you can jump to an expression)?
> > Iam not aware of these :) 
> >> What should this check do with indirect goto statements (it's a GCC 
> >> extension we support where you can jump to an expression)?
> >
> > Iam not aware of these :)
> 
> https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html
> (and a good reference on why these are interesting: 
> https://eli.thegreenplace.net/2012/07/12/computed-goto-for-efficient-dispatch-tables)
I would think that this is a special feature that will only be used if you know 
what you are doing. So it should be allowed with configuration. I am not sure 
about the default though. For now it is ignored.

HICPP has a rule on gotos as well, which states that only forward jumps are 
allowed.

I think that these different approaches to `goto` should land here sometime as 
different configurations.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:63
+
+  The usage of ``goto`` has been discouraged for a long time and is diagnosed
+  with this check.

I think will be good idea to reformulate this statement and its copy in 
documentation. //diagnosed with this check// is tautological for any check.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:22
+  if (getLangOpts().CPlusPlus)
+Finder->addMatcher(gotoStmt().bind("goto"), this);
+}

JonasToth wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > Are you planning to add the exception listed in the C++ Core Guideline? 
> > > It makes an explicit exception allowing you to jump forward out of a loop 
> > > construct.
> > What should this check do with indirect goto statements (it's a GCC 
> > extension we support where you can jump to an expression)?
> > 
> > Regardless, there should be a test for indirect gotos and jump forward out 
> > of loop constructs.
> > Are you planning to add the exception listed in the C++ Core Guideline? It 
> > makes an explicit exception allowing you to jump forward out of a loop 
> > construct.
> 
> I do plan for this. Because I dont have any experience with gotos I wanted to 
> do it in small steps.
> 
> > What should this check do with indirect goto statements (it's a GCC 
> > extension we support where you can jump to an expression)?
> Iam not aware of these :) 
>> What should this check do with indirect goto statements (it's a GCC 
>> extension we support where you can jump to an expression)?
>
> Iam not aware of these :)

https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html
(and a good reference on why these are interesting: 
https://eli.thegreenplace.net/2012/07/12/computed-goto-for-efficient-dispatch-tables)



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:27-28
+  const auto *Match = Result.Nodes.getNodeAs("goto");
+  diag(Match->getGotoLoc(), "'goto' is considered harmful; use high level "
+"programming constructs instead")
+  << Match->getSourceRange();

JonasToth wrote:
> aaron.ballman wrote:
> > I don't think this diagnostic really helps the user all that much. It 
> > doesn't say what's harmful about the goto, nor what high level programming 
> > construct to use to make it not harmful.
> I dont like the diagnostic too. But i dont know how to give a sensefull short 
> message.
> 
> Here Andrei Alexandrescu introduced some high level rules, maybe i the Rule 
> of Minimum Power could be a starting point?
> https://github.com/isocpp/CppCoreGuidelines/issues/19
> 
> > Good example for Rule of Minimum Power: goto is the most powerful looping 
> > construct (it can do everything while does, and a lot more such as jumping 
> > in the middle of a loop etc.) and the most unrecommended.
I think part of the issue here is that goto isn't harmful in all circumstances, 
but this check is going to tell users to remove the goto regardless of whether 
it's dangerous or not. That kind of broad prohibition suggests the diagnostic 
wording should be more along the lines of "avoid using 'goto' for flow control" 
or something along those lines. When you add the exception case(s) to the rule, 
you could reword to "this use of 'goto' for flow control is prohibited" (or 
similar).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:22
+  if (getLangOpts().CPlusPlus)
+Finder->addMatcher(gotoStmt().bind("goto"), this);
+}

aaron.ballman wrote:
> aaron.ballman wrote:
> > Are you planning to add the exception listed in the C++ Core Guideline? It 
> > makes an explicit exception allowing you to jump forward out of a loop 
> > construct.
> What should this check do with indirect goto statements (it's a GCC extension 
> we support where you can jump to an expression)?
> 
> Regardless, there should be a test for indirect gotos and jump forward out of 
> loop constructs.
> Are you planning to add the exception listed in the C++ Core Guideline? It 
> makes an explicit exception allowing you to jump forward out of a loop 
> construct.

I do plan for this. Because I dont have any experience with gotos I wanted to 
do it in small steps.

> What should this check do with indirect goto statements (it's a GCC extension 
> we support where you can jump to an expression)?
Iam not aware of these :) 



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:27-28
+  const auto *Match = Result.Nodes.getNodeAs("goto");
+  diag(Match->getGotoLoc(), "'goto' is considered harmful; use high level "
+"programming constructs instead")
+  << Match->getSourceRange();

aaron.ballman wrote:
> I don't think this diagnostic really helps the user all that much. It doesn't 
> say what's harmful about the goto, nor what high level programming construct 
> to use to make it not harmful.
I dont like the diagnostic too. But i dont know how to give a sensefull short 
message.

Here Andrei Alexandrescu introduced some high level rules, maybe i the Rule of 
Minimum Power could be a starting point?
https://github.com/isocpp/CppCoreGuidelines/issues/19

> Good example for Rule of Minimum Power: goto is the most powerful looping 
> construct (it can do everything while does, and a lot more such as jumping in 
> the middle of a loop etc.) and the most unrecommended.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D41815#969708, @JonasToth wrote:

> > Rather than add a warning for the labels, I would just add a note for the 
> > label when diagnosing the goto (since the goto has a single target).
>
> That might lead to existing labels without any gotos for them, does it? Maybe 
> the check could also diagnose labels without corresponding gotos, or does the 
> frontend already have something like this?


`-Wunused-label` already covers this scenario.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> Rather than add a warning for the labels, I would just add a note for the 
> label when diagnosing the goto (since the goto has a single target).

That might lead to existing labels without any gotos for them, does it? Maybe 
the check could also diagnose labels without corresponding gotos, or does the 
frontend already have something like this?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D41815#969673, @JonasToth wrote:

> In https://reviews.llvm.org/D41815#969626, @xazax.hun wrote:
>
> > A high level comment: while it is very easy to get all the gotos using 
> > grep, it is not so easy to get all the labels. So for this check to be 
> > complete, I think it would be useful to also find labels (possibly having a 
> > configuration option for that).
>
>
> Agreed. I will add warnings for jump labels too. Do you think a note where a 
> goto jumps to would be helpful too?


Rather than add a warning for the labels, I would just add a note for the label 
when diagnosing the goto (since the goto has a single target).




Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:22
+  if (getLangOpts().CPlusPlus)
+Finder->addMatcher(gotoStmt().bind("goto"), this);
+}

Are you planning to add the exception listed in the C++ Core Guideline? It 
makes an explicit exception allowing you to jump forward out of a loop 
construct.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:22
+  if (getLangOpts().CPlusPlus)
+Finder->addMatcher(gotoStmt().bind("goto"), this);
+}

aaron.ballman wrote:
> Are you planning to add the exception listed in the C++ Core Guideline? It 
> makes an explicit exception allowing you to jump forward out of a loop 
> construct.
What should this check do with indirect goto statements (it's a GCC extension 
we support where you can jump to an expression)?

Regardless, there should be a test for indirect gotos and jump forward out of 
loop constructs.



Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:27-28
+  const auto *Match = Result.Nodes.getNodeAs("goto");
+  diag(Match->getGotoLoc(), "'goto' is considered harmful; use high level "
+"programming constructs instead")
+  << Match->getSourceRange();

I don't think this diagnostic really helps the user all that much. It doesn't 
say what's harmful about the goto, nor what high level programming construct to 
use to make it not harmful.



Comment at: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst:6-7
+
+The usage of ``goto`` has been discouraged for a long time and is diagnosed
+with this check.
+

This doesn't really help the user understand what's bad about goto or why it 
should be diagnosed. You should expound a bit here.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D41815#969626, @xazax.hun wrote:

> A high level comment: while it is very easy to get all the gotos using grep, 
> it is not so easy to get all the labels. So for this check to be complete, I 
> think it would be useful to also find labels (possibly having a configuration 
> option for that).


Agreed. I will add warnings for jump labels too. Do you think a note where a 
goto jumps to would be helpful too?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 128902.
JonasToth added a comment.

- [Misc] reformat


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,7 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+int main() {
+jump_to_me:
+  goto jump_to_me;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'goto' is considered harmful; use high level programming constructs instead
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
Index: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
@@ -0,0 +1,11 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-goto
+
+cppcoreguidelines-avoid-goto
+
+
+The usage of ``goto`` has been discouraged for a long time and is diagnosed
+with this check.
+
+This check implements `ES.76 `_ 
+from the CppCoreGuidelines. For more information on why to avoid programming 
+with ``goto`` you can read the famous paper `A Case against the GO TO Statement. `_.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
 Improvements to clang-tidy
 --
 
+- New `cppcoreguidelines-avoid-goto
+  `_ check
+
+  The usage of ``goto`` has been discouraged for a long time and is diagnosed
+  with this check.
+
 - ...
 
 Improvements to include-fixer
Index: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
===
--- clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -11,6 +11,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
+#include "AvoidGotoCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"
@@ -35,6 +36,8 @@
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+CheckFactories.registerCheck(
+"cppcoreguidelines-avoid-goto");
 CheckFactories.registerCheck(
 "cppcoreguidelines-interfaces-global-init");
 CheckFactories.registerCheck("cppcoreguidelines-no-malloc");
Index: clang-tidy/cppcoreguidelines/CMakeLists.txt
===
--- clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -1,6 +1,7 @@
 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
+  AvoidGotoCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
   InterfacesGlobalInitCheck.cpp
   NoMallocCheck.cpp
Index: clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
@@ -0,0 +1,36 @@
+//===--- AvoidGotoCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDGOTOCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDGOTOCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {

[PATCH] D41815: [clang-tidy] implement check for goto

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

A high level comment: while it is very easy to get all the gotos using grep, it 
is not so easy to get all the labels. So for this check to be complete, I think 
it would be useful to also find labels (possibly having a configuration option 
for that).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 128899.
JonasToth added a comment.

- [Misc] emphasize goto in warning message


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,7 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+int main() {
+jump_to_me:
+  goto jump_to_me;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'goto' is considered harmful; use high level programming constructs instead
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
Index: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
@@ -0,0 +1,11 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-goto
+
+cppcoreguidelines-avoid-goto
+
+
+The usage of ``goto`` has been discouraged for a long time and is diagnosed
+with this check.
+
+This check implements `ES.76 `_ 
+from the CppCoreGuidelines. For more information on why to avoid programming 
+with ``goto`` you can read the famous paper `A Case against the GO TO Statement. `_.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
 Improvements to clang-tidy
 --
 
+- New `cppcoreguidelines-avoid-goto
+  `_ check
+
+  The usage of ``goto`` has been discouraged for a long time and is diagnosed
+  with this check.
+
 - ...
 
 Improvements to include-fixer
Index: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
===
--- clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -11,6 +11,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
+#include "AvoidGotoCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"
@@ -35,6 +36,8 @@
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+CheckFactories.registerCheck(
+"cppcoreguidelines-avoid-goto");
 CheckFactories.registerCheck(
 "cppcoreguidelines-interfaces-global-init");
 CheckFactories.registerCheck("cppcoreguidelines-no-malloc");
Index: clang-tidy/cppcoreguidelines/CMakeLists.txt
===
--- clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -1,6 +1,7 @@
 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
+  AvoidGotoCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
   InterfacesGlobalInitCheck.cpp
   NoMallocCheck.cpp
Index: clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
@@ -0,0 +1,36 @@
+//===--- AvoidGotoCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDGOTOCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDGOTOCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+names

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 128898.
JonasToth added a comment.

- fix typos and return type of main in test


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,7 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+int main() {
+jump_to_me:
+  goto jump_to_me;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: goto is considered harmful; use high level programming constructs instead
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
Index: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
@@ -0,0 +1,11 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-goto
+
+cppcoreguidelines-avoid-goto
+
+
+The usage of ``goto`` has been discouraged for a long time and is diagnosed
+with this check.
+
+This check implements `ES.76 `_ 
+from the CppCoreGuidelines. For more information on why to avoid programming 
+with ``goto`` you can read the famous paper `A Case against the GO TO Statement. `_.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
 Improvements to clang-tidy
 --
 
+- New `cppcoreguidelines-avoid-goto
+  `_ check
+
+  The usage of ``goto`` has been discouraged for a long time and is diagnosed
+  with this check.
+
 - ...
 
 Improvements to include-fixer
Index: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
===
--- clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -11,6 +11,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
+#include "AvoidGotoCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"
@@ -35,6 +36,8 @@
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+CheckFactories.registerCheck(
+"cppcoreguidelines-avoid-goto");
 CheckFactories.registerCheck(
 "cppcoreguidelines-interfaces-global-init");
 CheckFactories.registerCheck("cppcoreguidelines-no-malloc");
Index: clang-tidy/cppcoreguidelines/CMakeLists.txt
===
--- clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -1,6 +1,7 @@
 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
+  AvoidGotoCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
   InterfacesGlobalInitCheck.cpp
   NoMallocCheck.cpp
Index: clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
@@ -0,0 +1,36 @@
+//===--- AvoidGotoCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDGOTOCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDGOTOCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namesp

[PATCH] D41815: [clang-tidy] implement check for goto

[Jonas Toth via Phabricator via cfe-commits]

JonasToth created this revision.
JonasToth added reviewers: aaron.ballman, alexfh, hokein.
Herald added subscribers: cfe-commits, kbarton, xazax.hun, mgorny, nemanjai, 
klimek.

The usage of `goto` is discourage in C++ since forever. This check implements
a warning for every `goto`. Even though there are (rare) valid use cases for
`goto`, better high level constructs should be used.

`goto` is used sometimes in C programs to free resources at the end of 
functions in the case of errors. This pattern is better implemented with
RAII in C++.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815

Files:
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
  clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-avoid-goto.cpp

Index: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
@@ -0,0 +1,7 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-goto %t
+
+void main() {
+jump_to_me:
+  goto jump_to_me;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: goto is considered harmful; use high level programming constructs instead
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -52,6 +52,7 @@
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
+   cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
Index: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
@@ -0,0 +1,11 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-goto
+
+cppcoreguidelines-avoid-goto
+
+
+The usage of ``goto`` has been discouraged for a long time and is diagnosed
+with this check.
+
+This check implements `ES.76 `_ 
+from the CppCoreGuidelines. For more information on why to avoid programming 
+with ``goto`` you can read the famous paper `A Case against the GO TO Statement. `_.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
 Improvements to clang-tidy
 --
 
+- New `cppcoreguidelines-avoid-goto
+  `_ check
+
+  The usage of ``goto`` has been discouraged for a long time and is diagnosed
+  with this check.
+
 - ...
 
 Improvements to include-fixer
Index: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
===
--- clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -11,6 +11,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
+#include "AvoidGotoCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"
@@ -35,6 +36,8 @@
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+CheckFactories.registerCheck(
+"cppcoreguidelines-avoid-goto");
 CheckFactories.registerCheck(
 "cppcoreguidelines-interfaces-global-init");
 CheckFactories.registerCheck("cppcoreguidelines-no-malloc");
Index: clang-tidy/cppcoreguidelines/CMakeLists.txt
===
--- clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -1,6 +1,7 @@
 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
+  AvoidGotoCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
   InterfacesGlobalInitCheck.cpp
   NoMallocCheck.cpp
Index: clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
@@ -0,0 +1,36 @@
+//===--- AvoidGotoCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This