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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:20
+
+AST_MATCHER(GotoStmt, isForwardJumping) {
+
----------------
lebedev.ri wrote:
> It would be nice to have it in standard ASTMatchers, i believe it will be useful for `else-after-return` check.
> Though the ASTMatchers are stuck due to the doc-dumping script being 'broken' (see D41455)
Yes. The GNU extension goto does not have ASTMatcher yet, so i will pack these together in a review. What do you think how long the ASTMatcher issue will be there? Maybe it could be done after that check lands?


================
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);
+  }
----------------
aaron.ballman wrote:
> 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".
Yes, the new wording better. The note about the backward jump could be added in the `label defined here` note. I think distignuishing between backward jumps and forward jumps is still a good thing.

The forward jumps could come from a C code part where forward jumps are done for resource cleaning. So having a strong `prohibited` and a suggesting `avoid` diagnostic makes sense to me.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815