[PATCH] D41815: [clang-tidy] implement check for goto

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:22
+  if (getLangOpts().CPlusPlus)
+    Finder->addMatcher(gotoStmt().bind("goto"), this);
+}
----------------
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)


================
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();
----------------
JonasToth wrote:
> aaron.ballman wrote:
> > 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.
> I dont like the diagnostic too. But i dont know how to give a sensefull short message.
> 
> Here Andrei Alexandrescu introduced some high level rules, maybe i the Rule of Minimum Power could be a starting point?
> https://github.com/isocpp/CppCoreGuidelines/issues/19
> 
> > Good example for Rule of Minimum Power: goto is the most powerful looping construct (it can do everything while does, and a lot more such as jumping in the middle of a loop etc.) and the most unrecommended.
I think part of the issue here is that goto isn't harmful in all circumstances, but this check is going to tell users to remove the goto regardless of whether it's dangerous or not. That kind of broad prohibition suggests the diagnostic wording should be more along the lines of "avoid using 'goto' for flow control" or something along those lines. When you add the exception case(s) to the rule, you could reword to "this use of 'goto' for flow control is prohibited" (or similar).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815