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&id=130115#toc
Repository:
rCTE Clang Tools E
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
clang-tidy/cppc
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");
-CheckFactories.registerC
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
clang-tidy/cppcoreguide
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
clang-tidy/cpp
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
clan
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 longe
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
https://revie
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
clang-tidy/cppco
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
clang-tidy/cppcoregui
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 wrote:
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 wro
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 i
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
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
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
clang-tidy/c
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 false-pos
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 doesn
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
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 ide
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:
clang-tidy/cppcoreguideli
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 be
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-
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
> >
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 lo
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.
Though
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 a
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 =
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
ju
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:
> > > aaron
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
documentation.
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:
> > > A
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
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 go
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 withou
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 c
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 (p
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
clang-tidy/cppcoreguideli
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:
rC
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
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
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 the
42 matches
Mail list logo
