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-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,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 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es76-avoid-goto>`_
+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. <https://www.cs.utexas.edu/users/EWD/ewd02xx/EWD215.PDF>`_.
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
Improvements to clang-tidy
--------------------------
+- New `cppcoreguidelines-avoid-goto
+ <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-goto.html>`_ 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<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,58 @@
+//===--- 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) {
+ // Check if the 'goto' is used for control flow other then 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
