[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:20
+
+AST_MATCHER(GotoStmt, isForwardJumping) {
+
----------------
JonasToth wrote:
> lebedev.ri wrote:
> > JonasToth wrote:
> > > 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?
> > > Maybe it could be done after that check lands?
> > 
> > Yes, absolutely. I did not meant that as a blocker here.
> On my todo list.
I'm not certain adding this to the AST matchers is critical. For instance, we may want to, instead, consider adding relational operators for source locations and then expose a generic facility for getting source location information out of AST nodes.


================
Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:26
+  // TODO: This check does not recognize `IndirectGotoStmt` which is a
+  // GNU extension. These must be matched separatly and a ASTMatcher
+  // is currently missing for them. Adding this matcher and moving
----------------
separatly -> separately
a ASTMatcher -> an AST matcher


================
Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:28-29
+  // is currently missing for them. Adding this matcher and moving
+  // `isForwardJumping` (as requested by LebedevRI) to ASTMatchers
+  // will be done together.
+
----------------
We don't usually put names into the source code and given the questions about adding that matcher, I'd say it's better to remove the promise that this will become a generic one.


================
Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:43
+
+  // Backward jumps are diagnosed in all language modes. Forward jumps
+  // are sometimes required in C to free resources or do other clean-up
----------------
I think that this check should be C++ only. C makes far more use of `goto`, and backwards jumps are not always bad there (they don't have to consider things like destructors or RAII like you do in C++).

Esp since this is a check for the C++ core guidelines and HICPP (both are C++ standards). 


================
Comment at: docs/ReleaseNotes.rst:67-68
 
+- New `cppcoreguidelines-avoid-goto
+  <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-goto.html>`_ check
+
----------------
I think you should also add the HICPP changes as well, given that this check also covers that rule.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41815