[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D122895#3511646 <https://reviews.llvm.org/D122895#3511646>, @dexonsmith wrote:

> In D122895#3511611 <https://reviews.llvm.org/D122895#3511611>, @dexonsmith wrote:
>
>> Previously, `-Wstrict-prototypes` was useful for preventing actual bugs in code.
>
> For example, it's important to have a warning that catches code like this:
>
>   void f1(void (^block)());
>   
>   void f2(void) {
>     f1(^(int x) { /* do something with x */ });
>   }
>
> without triggering in cases that are pedantic.
>
> (It also seems unfortunate to regress the false positive rate of this diagnostic before `-fstrict-prototypes` is available.)

`-fno-knr-functions` is available already today in trunk, so I'm not certain I understand your concern there (or were you unaware we changed the flag name perhaps?).

Your code example is interesting though as I would expect that to trigger `-Wdeprecated-non-prototype` as well as `-Wstrict-prototypes`. I'd expect the pedantic warning to complain about the block declaration because it specifies no prototype, and I'd expect the changes behavior warning because that code will break in C2x.

> Is it necessary to make -Wstrict-prototypes weaker in order to move the newer more pedantic cases to a different name?

This diagnostic is *extremely* difficult for a whole host of reasons. 1) we need to diagnose function *types* without a prototype, but you form a function type when declaring or defining a function in addition to just using the type in a typedef or a variable declaration. 2) we need to handle function definitions differently; that's the only point at which we know whether the user is defining a function with an identifier list which will change behavior in C2x, 3) we need to handle function declaration merging (which happens for definitions as well as redeclarations). So there's this very convoluted dance where some cases are handled in `Sema::GetFullTypeForDeclarator()` (this is the only place we trigger the pedantic warning from), some are handled in `Sema::ActOnFinishFunctionBody()`, and some are handled in `Sema::MergeFunctionDecl()`. Trying to weaken `-Wstrict-prototypes` specifically means having to make the decision at the time when the only information we have is what we can glean from the declarator.

> Oh, I thought that would catch bugs like this:
>
>   void longfunctionname(){}
>   void longfunctionnames(int x){ /* do something with x */ }
>   
>   void g(void) {
>     longfunctionname(7); // oops, meant to call longfunctionnames().
>   }
>
> but if it's entirely pedantic (if the call to longfunctionname(7) would fail to compile) then I agree it'd be better to silence it unless someone asks for -pedantic.

The call to `longfunctionname(7)` would be rejected in C2x and accepted in earlier modes, which is why it gets flagged `warning: passing arguments to 'longfunctionname' without a prototype is deprecated in all versions of C and is not supported in C2x`. It's worth noting that `-Wstrict-prototypes` is now in `-pedantic` explicitly (it shifted from being a `Warning` to an `Extension` because it is now a pedantic warning).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122895/new/

https://reviews.llvm.org/D122895