[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<GotoStmt>("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