cppcoreguidelines-avoid-goto doesn't work on C code

[Quuxplusone]

Bugzilla Link	PR47721
Status	NEW
Importance	P enhancement
Reported by	Gabriel Ravier (gabravier@gmail.com)
Reported on	2020-10-04 05:54:41 -0700
Last modified on	2020-10-06 10:19:22 -0700
Version	unspecified
Hardware	PC Linux
CC	alexfh@google.com, development@jonas-toth.eu, djasper@google.com, eugene.zelenko@gmail.com, klimek@google.com, N.James93@hotmail.co.uk
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

void f()
{
    goto label;
label:;
}

If compiled as C++ and then checked with `--checks=cppcoreguidelines-avoid-
goto`, it will flag this code. If compiled as C and then checked with the same
flag, it will not. This is most likely due to the check in clang-tools-
extra/clang-tidy/cppcoreguidelines/AvoidGotoCheck.h for C++ :

  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
    return LangOpts.CPlusPlus;
  }

Is this too complicated to implement for C, or is this simply an oversight ? I
would appreciate having such checks in C mode without having to pretend to
clang-tidy all my code is C++.

See also proof of this on Godbolt here : https://godbolt.org/z/Y9co3h (-xc is
to make it so that clang-tidy assumes the code is C)

[Quuxplusone]

Sure, similar rules make sense in C too.

However, it's hard to expect C++ related check to work in C code. Probably common check code should be used of both languages implementation but with different check names.

[Quuxplusone]

Most cppcoreguidelines checks seem to work on C where it makes sense to. For example, checks like cppcoreguidelines-avoid-magic-numbers, cppcoreguidelines-avoid-non-const-global-variables will operate properly on C code. Of course, I don't mind having an alias with a name that doesn't have "cpp" in the name, so that it is more obvious the check works with C, but IMHO voluntarily sabotaging checks against being used with C code because they have a name that makes them seem like they only work with C++ will do nothing but annoy people (such as me) who will have to either spend a long time either trying to figure out why "that clang-tidy check for goto just won't work on my C code" before digging into the source and seeing C blacklisted, or else have to find a different name to add onto their command line/configuration file.

[Quuxplusone]

Hi Gabriel,

the intention of the check was to be limited to C++. goto has more uses in C and the check is intentionally a C++ coding standard enforcement.

See this comment in the review: https://reviews.llvm.org/D41815#inline-367308

Your comment on C/C++ analysis: The problem is, that we want to provide good checks whenever we can. In the goto example, it is not that obvious how to differentiate between good use of goto and bad use in C. So rather not introduce false positives that lead to deactivation of the analysis in general.

We lack strong C support and most checks are focussed on C++, that is right! But the truth is, that C is way harder to figure out what is good and what is bad. :/ C++ provides better constructs that can replace legitimate uses of sometimes-bad-constructs and then forbid the sometimes-bad-constructs overall. I think, that is one of the reasons why clang-tidy is much more hesitant for C code. (we still try to enable C whenever feasible, and some checks are only C, e.g. some cert-* checks).

[Quuxplusone]

I guess that is a good point. I must say though, rn to make sure my code doesn't use goto, I just do -Dgoto='_Static_assert(0, "do not use goto")' but that's a total hack, so I was searching for something that was at least somewhat better. Even if it was a check that would sometimes flag good usage of goto, a clang-tidy check like the one here still seemed better. Not gonna try to push to get a broken check into clang-tidy if you consider it to be broken for C code, though.

[Quuxplusone]

It might be completely ok to have an optional C mode that would allow only forward-gotos or something like this / configurable strictness.

If you have time, a patch would be welcome ;) Otherwise it lands on the todo list in the back of my head.

[Quuxplusone]

I was always of the opinion that in c++ we have features to avoid the need to use goto along with features that don't work nicely with goto, The main ones being destructors. As these features don't exist in 'C', goto is the best way implement destruction of resources while reducing code duplication. This alone should be good enough reason not to have this check run on 'C' translation units.

I do agree maybe a separate check could be made that would only flag certain use cases of goto. However, as I never use 'C', I wouldn't know the best heuristics to look for with suspicious goto statements.