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-msc30-c>
cert-msc50-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) <cert-oop11-cpp>
+ cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) <cppcoreguidelines-c-copy-assignment-signature>
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 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es76-avoid-goto>`_
+from the CppCoreGuidelines and
+`6.3.1 from High Integrity C++ <http://www.codingstandard.com/rule/6-3-1-ensure-that-the-labels-for-a-jump-statement-or-a-switch-condition-appear-later-in-the-same-or-an-enclosing-block/>`_.
+
+For more information on why to avoid programming
+with ``goto`` you can read the famous paper `A Case against the GO TO Statement. <https://www.cs.utexas.edu/users/EWD/ewd02xx/EWD215.PDF>`_.
+
+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
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -64,6 +64,12 @@
object is statically initialized with a ``constexpr`` constructor or has no
explicit constructor.
+- New `cppcoreguidelines-avoid-goto
+ <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-goto.html>`_ check
+
+ 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.
+
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<AvoidGotoCheck>(
+ "cppcoreguidelines-avoid-goto");
CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
"cppcoreguidelines-interfaces-global-init");
CheckFactories.registerCheck<NoMallocCheck>("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 {
+
+/// The usage of `goto` has been discouraged for a long time and is diagnosed
+/// with this check.
+///
+/// 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/AvoidGotoCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
@@ -0,0 +1,63 @@
+//===--- 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) {
+ // 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
+ // `isForwardJumping` (as requested by LebedevRI) to ASTMatchers
+ // will be done together.
+
+ // 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))))
+ .bind("nested-loop");
+
+ Finder->addMatcher(gotoStmt(unless(isForwardJumping())).bind("backward-goto"),
+ this);
+ Finder->addMatcher(
+ gotoStmt(unless(hasAncestor(NestedLoop))).bind("flow-goto"), this);
+}
+
+void AvoidGotoCheck::check(const MatchFinder::MatchResult &Result) {
+ if (const auto *BackGoto =
+ Result.Nodes.getNodeAs<GotoStmt>("backward-goto")) {
+ diag(BackGoto->getGotoLoc(), "do not jump backwards with 'goto'")
+ << BackGoto->getSourceRange();
+ diag(BackGoto->getLabel()->getLocStart(), "label defined here",
+ DiagnosticIDs::Note);
+ }
+
+ // The guidelines allow 'goto' only for jumps out of a nested loop.
+ if (const auto *Flow = Result.Nodes.getNodeAs<GotoStmt>("flow-goto")) {
+ diag(Flow->getGotoLoc(), "avoid using 'goto' for flow control")
+ << Flow->getSourceRange();
+ }
+} // namespace cppcoreguidelines
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
