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/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/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/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/hicpp/HICPPTidyModule.cpp =================================================================== --- clang-tidy/hicpp/HICPPTidyModule.cpp +++ clang-tidy/hicpp/HICPPTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" #include "../bugprone/UseAfterMoveCheck.h" +#include "../cppcoreguidelines/AvoidGotoCheck.h" #include "../cppcoreguidelines/NoMallocCheck.h" #include "../cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.h" #include "../cppcoreguidelines/ProTypeMemberInitCheck.h" @@ -45,6 +46,8 @@ class HICPPModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( + "hicpp-avoid-goto"); CheckFactories.registerCheck( "hicpp-braces-around-statements"); CheckFactories.registerCheck( Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -57,6 +57,13 @@ Improvements to clang-tidy -------------------------- +- New `cppcoreguidelines-avoid-goto + `_ check + + The usage of ``goto`` for control flow is error prone and should be replaced + with looping constructs. Every backward jump is rejected. Forward jumps are + only allowed in nested loops. + - New `fuchsia-statically-constructed-objects `_ check @@ -64,6 +71,11 @@ object is statically initialized with a ``constexpr`` constructor or has no explicit constructor. +- New alias `hicpp-avoid-goto + `_ to + `cppcoreguidelines-avoid-goto `_ + added. + Improvements to include-fixer ----------------------------- Index: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst =================================================================== --- docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst +++ 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(); + +All other uses of ``goto`` are diagnosed in `C++`. Index: docs/clang-tidy/checks/hicpp-avoid-goto.rst =================================================================== --- docs/clang-tidy/checks/hicpp-avoid-goto.rst +++ 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++ `_ +requires that ``goto`` only skips parts of a block and is not used for other +reasons. + +Both coding guidelines implement the same exception to the usage of ``goto``. 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: test/clang-tidy/cppcoreguidelines-avoid-goto.cpp =================================================================== --- test/clang-tidy/cppcoreguidelines-avoid-goto.cpp +++ 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 + } + } +}