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

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE322626: [clang-tidy] implement check for goto (authored by JonasToth, committed by ). Changed prior to commit: https://reviews.llvm.org/D41815?vs=130113=130115#toc Repository: rCTE Clang Tools

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

JonasToth updated this revision to Diff 130113. JonasToth marked an inline comment as done. JonasToth added a comment. - [Misc] remove spurious format Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41815 Files: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a minor formatting nit. Comment at: clang-tidy/hicpp/HICPPTidyModule.cpp:57 "hicpp-exception-baseclass"); -

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

JonasToth updated this revision to Diff 129901. JonasToth added a comment. - last nits i found Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41815 Files: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp clang-tidy/cppcoreguidelines/AvoidGotoCheck.h

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

JonasToth updated this revision to Diff 129900. JonasToth added a comment. - remove spurious whitespace Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41815 Files: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp clang-tidy/cppcoreguidelines/AvoidGotoCheck.h

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

JonasToth updated this revision to Diff 129899. JonasToth marked 3 inline comments as done. JonasToth added a comment. - hicpp alias added - update documentation Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41815 Files: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp

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

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.h:19-20 + +/// The usage of `goto` has been discouraged for a long time and is diagnosed +/// with this check. +/// This comment is a bit out of date now that it no

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

JonasToth updated this revision to Diff 129868. JonasToth added a comment. - doc clarified that check is c++ only - add missing tests for `do` and `range-for`, not all combinations done, but every loop construct is used as outer and inner loop Repository: rCTE Clang Tools Extra

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

JonasToth updated this revision to Diff 129865. JonasToth marked an inline comment as done. JonasToth added a comment. - [Fix] bug with language mode Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41815 Files: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp

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

JonasToth updated this revision to Diff 129863. JonasToth marked 4 inline comments as done. JonasToth added a comment. - address review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41815 Files: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp

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

JonasToth added inline comments. 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 aaron.ballman

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

JonasToth marked 3 inline comments as done. JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:21 +AST_MATCHER(GotoStmt, isForwardJumping) { + return Node.getLocStart() < Node.getLabel()->getLocStart(); +} lebedev.ri

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

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

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

lebedev.ri added inline comments. Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:21 +AST_MATCHER(GotoStmt, isForwardJumping) { + return Node.getLocStart() < Node.getLabel()->getLocStart(); +} JonasToth wrote: > lebedev.ri wrote: > > Hm, on a second

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

JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:21 +AST_MATCHER(GotoStmt, isForwardJumping) { + return Node.getLocStart() < Node.getLabel()->getLocStart(); +} lebedev.ri wrote: > Hm, on a second thought, i think this

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

JonasToth updated this revision to Diff 129760. JonasToth added a comment. - add edgecase test from roman Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41815 Files: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp clang-tidy/cppcoreguidelines/AvoidGotoCheck.h

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

lebedev.ri added inline comments. Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:21 +AST_MATCHER(GotoStmt, isForwardJumping) { + return Node.getLocStart() < Node.getLabel()->getLocStart(); +} Hm, on a second thought, i think this will have

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

Eugene.Zelenko added inline comments. Comment at: docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst:6-7 + +The usage of ``goto`` has been discouraged for a long time and is diagnosed +with this check. + JonasToth wrote: > aaron.ballman wrote: > > This

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

JonasToth updated this revision to Diff 129684. JonasToth added a comment. - simplified the code and merged diagnostics Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41815 Files: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp clang-tidy/cppcoreguidelines/AvoidGotoCheck.h

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

JonasToth marked 5 inline comments as done. JonasToth added inline comments. Comment at: docs/ReleaseNotes.rst:63 + + The usage of ``goto`` has been discouraged for a long time and is diagnosed + with this check. Eugene.Zelenko wrote: > I think will be good

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

JonasToth updated this revision to Diff 129677. JonasToth marked 8 inline comments as done. JonasToth added a comment. - address review comments - add better documentation with code examples Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41815 Files:

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

JonasToth marked 3 inline comments as done. JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:20 + +AST_MATCHER(GotoStmt, isForwardJumping) { + lebedev.ri wrote: > JonasToth wrote: > > lebedev.ri wrote: > > > It would

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

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

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

lebedev.ri added inline comments. Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:20 + +AST_MATCHER(GotoStmt, isForwardJumping) { + JonasToth wrote: > lebedev.ri wrote: > > It would be nice to have it in standard ASTMatchers, i believe it will be >

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

JonasToth added a comment. In https://reviews.llvm.org/D41815#973265, @lebedev.ri wrote: > In https://reviews.llvm.org/D41815#973260, @JonasToth wrote: > > > - check that jumps will only be forward. AFAIK that is required in all > > sensefull usecases of goto, is it? > > > You could implement

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

lebedev.ri added inline comments. Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:20 + +AST_MATCHER(GotoStmt, isForwardJumping) { + It would be nice to have it in standard ASTMatchers, i believe it will be useful for `else-after-return` check.

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

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

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

lebedev.ri added a comment. In https://reviews.llvm.org/D41815#973260, @JonasToth wrote: > - check that jumps will only be forward. AFAIK that is required in all > sensefull usecases of goto, is it? You could implement loops/recursion with goto, something like: const int end = 10; int i

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

JonasToth updated this revision to Diff 129433. JonasToth added a comment. 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

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

JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:22 + if (getLangOpts().CPlusPlus) +Finder->addMatcher(gotoStmt().bind("goto"), this); +} aaron.ballman wrote: > JonasToth wrote: > > aaron.ballman wrote: > > >

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

Eugene.Zelenko added inline comments. Comment at: docs/ReleaseNotes.rst:63 + + The usage of ``goto`` has been discouraged for a long time and is diagnosed + with this check. I think will be good idea to reformulate this statement and its copy in

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

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: > > >

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

JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:22 + if (getLangOpts().CPlusPlus) +Finder->addMatcher(gotoStmt().bind("goto"), this); +} aaron.ballman wrote: > aaron.ballman wrote: > > Are you planning to add the

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

aaron.ballman added a comment. In https://reviews.llvm.org/D41815#969708, @JonasToth wrote: > > Rather than add a warning for the labels, I would just add a note for the > > label when diagnosing the goto (since the goto has a single target). > > That might lead to existing labels without any

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

JonasToth added a comment. > Rather than add a warning for the labels, I would just add a note for the > label when diagnosing the goto (since the goto has a single target). That might lead to existing labels without any gotos for them, does it? Maybe the check could also diagnose labels

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

aaron.ballman added a comment. In https://reviews.llvm.org/D41815#969673, @JonasToth wrote: > In https://reviews.llvm.org/D41815#969626, @xazax.hun wrote: > > > A high level comment: while it is very easy to get all the gotos using > > grep, it is not so easy to get all the labels. So for this

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

JonasToth added a comment. In https://reviews.llvm.org/D41815#969626, @xazax.hun wrote: > A high level comment: while it is very easy to get all the gotos using grep, > it is not so easy to get all the labels. So for this check to be complete, I > think it would be useful to also find labels

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

JonasToth updated this revision to Diff 128902. JonasToth added a comment. - [Misc] reformat Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41815 Files: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp clang-tidy/cppcoreguidelines/AvoidGotoCheck.h

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

xazax.hun added a comment. A high level comment: while it is very easy to get all the gotos using grep, it is not so easy to get all the labels. So for this check to be complete, I think it would be useful to also find labels (possibly having a configuration option for that). Repository:

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

JonasToth updated this revision to Diff 128899. JonasToth added a comment. - [Misc] emphasize goto in warning message Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41815 Files: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp clang-tidy/cppcoreguidelines/AvoidGotoCheck.h

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

JonasToth updated this revision to Diff 128898. JonasToth added a comment. - fix typos and return type of main in test Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41815 Files: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp clang-tidy/cppcoreguidelines/AvoidGotoCheck.h

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

JonasToth created this revision. JonasToth added reviewers: aaron.ballman, alexfh, hokein. Herald added subscribers: cfe-commits, kbarton, xazax.hun, mgorny, nemanjai, klimek. The usage of `goto` is discourage in C++ since forever. This check implements a warning for every `goto`. Even though