Page MenuHomePhabricator

[clang-tidy] implement check for goto
ClosedPublic

Authored by JonasToth on Jan 8 2018, 1:38 AM.

Details

Summary

The usage of goto is discourage in C++ since forever. This check implements
a warning for every goto. Even though there are (rare) valid use cases for
goto, better high level constructs should be used.

goto is used sometimes in C programs to free resources at the end of
functions in the case of errors. This pattern is better implemented with
RAII in C++.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
JonasToth updated this revision to Diff 128898.Jan 8 2018, 1:54 AM
  • fix typos and return type of main in test
JonasToth updated this revision to Diff 128899.Jan 8 2018, 1:55 AM
  • [Misc] emphasize goto in warning message

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).

JonasToth updated this revision to Diff 128902.Jan 8 2018, 2:47 AM
  • [Misc] reformat

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).

Agreed. I will add warnings for jump labels too. Do you think a note where a goto jumps to would be helpful too?

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).

Agreed. I will add warnings for jump labels too. Do you think a note where a goto jumps to would be helpful too?

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).

clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
23

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.

23

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.

28–29

I don't think this diagnostic really helps the user all that much. It doesn't say what's harmful about the goto, nor what high level programming construct to use to make it not harmful.

docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
7–8

This doesn't really help the user understand what's bad about goto or why it should be diagnosed. You should expound a bit here.

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 without corresponding gotos, or does the frontend already have something like this?

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 without corresponding gotos, or does the frontend already have something like this?

-Wunused-label already covers this scenario.

JonasToth added inline comments.Jan 8 2018, 5:58 AM
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
23

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

28–29

I dont like the diagnostic too. But i dont know how to give a sensefull short message.

Here Andrei Alexandrescu introduced some high level rules, maybe i the Rule of Minimum Power could be a starting point?
https://github.com/isocpp/CppCoreGuidelines/issues/19

Good example for Rule of Minimum Power: goto is the most powerful looping construct (it can do everything while does, and a lot more such as jumping in the middle of a loop etc.) and the most unrecommended.

aaron.ballman added inline comments.Jan 8 2018, 6:04 AM
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
23

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)

28–29

I think part of the issue here is that goto isn't harmful in all circumstances, but this check is going to tell users to remove the goto regardless of whether it's dangerous or not. That kind of broad prohibition suggests the diagnostic wording should be more along the lines of "avoid using 'goto' for flow control" or something along those lines. When you add the exception case(s) to the rule, you could reword to "this use of 'goto' for flow control is prohibited" (or similar).

Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
63

I think will be good idea to reformulate this statement and its copy in documentation. diagnosed with this check is tautological for any check.

jbcoe added a subscriber: jbcoe.Jan 11 2018, 12:35 AM
JonasToth added inline comments.Jan 11 2018, 4:24 AM
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
23

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.

JonasToth updated this revision to Diff 129433.Jan 11 2018, 4:53 AM

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.

Some test cases for all combinations are missing, i can add those once you
agree that the functionality change is indeed ok.

  • 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 = 0;
assert(i < end);

begin:
<do stuff>
i++
if(i < end)
  goto begin;

// end

But it really should be done with normal for(), or while(), so i think it would make sense to diagnose those.

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.

clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
22

Can remove the spurious newline.

23

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.

27

s/then/than

42–54

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".

lebedev.ri added inline comments.Jan 11 2018, 11:39 AM
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
21

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)

  • 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 = 0;
assert(i < end);

begin:
<do stuff>
i++
if(i < end)
  goto begin;

// end

But it really should be done with normal for(), or while(), so i think it would make sense to diagnose those.

That check is specifically targeting these code constellations to be bad practice. As you said, using the looping constructs is the way to go :)

JonasToth added inline comments.Jan 11 2018, 1:57 PM
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
21

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?

42–54

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.

lebedev.ri added inline comments.Jan 11 2018, 1:58 PM
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
21

Maybe it could be done after that check lands?

Yes, absolutely. I did not meant that as a blocker here.

JonasToth marked 3 inline comments as done.Jan 12 2018, 11:10 AM
JonasToth added inline comments.
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
21

On my todo list.

JonasToth marked 2 inline comments as done.Jan 12 2018, 11:10 AM
JonasToth marked 8 inline comments as done.
  • address review comments
  • add better documentation with code examples
JonasToth marked 5 inline comments as done.Jan 12 2018, 12:24 PM
JonasToth added inline comments.
docs/ReleaseNotes.rst
63

Reformulated, is it ok now?

docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
7–8

Is it better now?

  • simplified the code and merged diagnostics
docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst
7–8

Much better now! Thank you for fix!

JonasToth marked 5 inline comments as done.Jan 12 2018, 12:59 PM
lebedev.ri added inline comments.Jan 12 2018, 11:22 PM
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
22

Hm, on a second thought, i think this will have false-positive if the label and the goto are on the same line, like

goto label; ; label: ;

I wonder we could easily compare accounting for the position in the line, or it is not worth the extra complexity.

JonasToth updated this revision to Diff 129760.Jan 13 2018, 6:40 AM
  • add edgecase test from roman
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
22

Iam not sure what you mean. Is the concern that the goto does not actually skip something?
If yes it is still worth it, because its not necessary to have the goto then.

The < on the locations should always be sensefull because AFAIK its at byte level (or something similar).

Your example is diagnosed correctly.

lebedev.ri added inline comments.Jan 13 2018, 7:12 AM
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
22

The < on the locations should always be sensefull because AFAIK its at byte level (or something similar).

Ah, great then. I had some recollections of having some issues with that before, but it makes sense that it would just work.

aaron.ballman added inline comments.Jan 13 2018, 7:55 AM
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
21

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.

27

separatly -> separately
a ASTMatcher -> an AST matcher

29–30

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.

44

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).

docs/ReleaseNotes.rst
74–75

I think you should also add the HICPP changes as well, given that this check also covers that rule.

JonasToth marked 3 inline comments as done.Jan 13 2018, 8:23 AM
JonasToth added inline comments.
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
22

I dont know the internals, i just assume because it works :)

JonasToth marked 2 inline comments as done.Jan 13 2018, 8:24 AM
JonasToth updated this revision to Diff 129863.Jan 15 2018, 8:27 AM
JonasToth marked 4 inline comments as done.
  • address review comments
clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp
44

Ok. I merged the matchers too.

docs/ReleaseNotes.rst
74–75

I think Only forward jumps in nested loops are accepted. covers it, but i reformulated it.

JonasToth updated this revision to Diff 129865.Jan 15 2018, 8:30 AM
JonasToth marked an inline comment as done.
  • [Fix] bug with language mode
JonasToth updated this revision to Diff 129868.Jan 15 2018, 8:43 AM
  • 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
aaron.ballman added inline comments.Jan 15 2018, 12:23 PM
clang-tidy/cppcoreguidelines/AvoidGotoCheck.h
20–21

This comment is a bit out of date now that it no longer diagnoses all uses of goto.

JonasToth marked 3 inline comments as done.
  • hicpp alias added
  • update documentation
  • remove spurious whitespace
  • last nits i found
aaron.ballman accepted this revision.Jan 16 2018, 8:46 AM

LGTM aside from a minor formatting nit.

clang-tidy/hicpp/HICPPTidyModule.cpp
57

Spurious formatting change -- feel free to commit separately (no review required) if you want to fix it.

This revision is now accepted and ready to land.Jan 16 2018, 8:46 AM
JonasToth updated this revision to Diff 130113.Jan 17 2018, 2:22 AM
JonasToth marked an inline comment as done.
  • [Misc] remove spurious format
This revision was automatically updated to reflect the committed changes.