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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits