[PATCH] D66919: Warn about zero-parameter K&R definitions in -Wstrict-prototypes

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D66919#1651108 <https://reviews.llvm.org/D66919#1651108>, @dexonsmith wrote:

> we just don't warn on non-prototype defining declarations, where the meaning is unambiguous:
>
>   void foo() {}
>


"Meaning" is a difficult term. What we know is that this is a K&R-style definition, and does not define a prototype. So one would expect `-Wstrict-prototypes` to warn about this.

If you look at the section about function calls in the C11 standard (6.5.2.2), you can see that it distinguishes "Constraints" and "Semantics". Constraints are only placed on calls to prototype functions: "If the expression that denotes the called function has a type that includes a prototype, the number of arguments shall agree with the number of parameters. Each argument shall have a type such that its value may be assigned to an object with the unqualified version of the type of its corresponding parameter." Only in the semantics section we find something regarding calls to non-prototype functions: "If the expression that denotes the called function has a type that does not include a prototype, [...]. If the number of arguments does not equal the number of parameters, the behavior is undefined." So one is a compiler error, the other a runtime error (at best).

For me this warning is about usage of an "obsolescent" feature (6.11.6 and 6.11.7), and thus should warn even when we can warn about wrong usage at call sites. Note that the warning is off by default and neither enabled by `-Wall` nor `-Wextra`.

In D66919#1651259 <https://reviews.llvm.org/D66919#1651259>, @aaron.ballman wrote:

> There are two different warnings, and perhaps we're speaking about different ones. We have a warning about not having a prototype (warning: this function declaration is not a prototype) and we have a warning about not seeing a preceding prototype (warning: this old-style function definition is not preceded by a prototype). I think this patch deals with the latter.


There is also `-Wmissing-prototypes`, which could have been named better. It warns about a definition (never a declaration) that doesn't have a preceding prototype declaration. It warns when there are preceding declarations that do not provide a prototype, and also when there is no preceding declaration at all, even if the definition itself does provide a prototype. It doesn't fire on `static` or `inline` definitions. I would describe the purpose of this warning as finding functions that should be declared `static`.

This warning (`-Wstrict-prototypes`) is rather straightforward: it warns on old-style (K&R) function declarators and enforces the usage of prototype declarations. (Well, with the caveat that among multiple non-definition declarations, only the first declaration needs to provide said prototype.) The purpose of this warning is to detect (perhaps unintentional) usage of the old-style unsafe declarator syntax.

So the warnings have some overlap, but I see their intention as different.

>> Given my understanding, then the only corner case that's left is when we *do* see the definition.
> 
> Yeah, and we already handle that situation with an un-ignorable warning: https://godbolt.org/z/TPklNE

A non-prototype definition could be inline in a header. Now we have a warning that detects when we call the function with the wrong number of arguments, but users of the header might use a different compiler and not see a warning: the compiler is not required to diagnose this, as it's undefined behavior. Now if Clang warns me that this isn't a prototype, I'll fix it and users of the header with the hypothetical other compiler now get an error if they use it wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66919