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