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

Fri Aug 30 14:16:32 PDT 2019

aaronpuchert added a comment.

We had a discussion on IRC yesterday and @aaron.ballman pointed out that the K&R syntax has been considered "obsolescent" (= deprecated) since C89, 30 years ago. He added that there are thoughts within the standards committee about removing the syntax entirely from C2x.

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

> I would suggest `-Werror`-by-default since the compiler knows it's incorrect code.

Do we have any precedence for a warning that's treated as error by default? I haven't seen that before, that's why I'm asking. Even `-Wreturn-type` isn't treated as error by default.

> All I'm suggesting is that the compiler knows that the call is wrong, so it should error (by-default).

K&R allows weird stuff, so I'm really not sure it's necessarily wrong. To my knowledge the `...` varargs syntax was only introduced with prototypes, and the K&R version of printf was "implicitly vararg". Meaning that the additional parameters weren't mentioned in the function header at all, and scraped from the stack using the `va_*` macros as it's done today.

Also by that argument we shouldn't emit `-Wstrict-prototypes` at all on definitions, because clearly the compiler can also do type checking then. In fact, we actually do that to some extent: instead of default-promoting the call arguments, we default-promote the parameter types and build a `FunctionProtoType`, so that calls work as if there had been a proper prototype. This has the odd side effect of causing different behavior depending on whether the compiler sees the definition of a function, as @aaron.ballman noted. The example we looked at was

  int f();
  int g() { return f(1.0); }
  int f(x) int x; {}
  int h() { return f(1.0); }

One might think that we emit the same code for `g` and `h`, but we don't!

> After adding that, my feeling is that diagnosing a missing prototype on a defining declaration would be a style nitpick that might fit better in clang-tidy.

It's not a stylistic issue, prototype and non-prototype declarations are semantically very different. C has **no notion of argument checking for non-prototypes** at all. The `identifier-list` of a K&R definition is essentially private.

Of course we can impose our own semantics for non-prototypes, but we can also just nudge people into using standard syntax that has been available for many decades that guarantees them proper argument checks, both regarding number and types of arguments.

> IIRC, when we rolled out `-Wstrict-prototypes` we explicitly excluded this case since it hit a lot of code without finding bugs.

The GCC people didn't, so it can't be that bad. Also we already warn on block definitions with zero parameters for some reason. Lastly, the fix is very easy: just add `void`. With some `-fixit` magic it might just be a matter of running Clang over the entire code base once and you're done with it.

We don't only warn on actual bugs, but also on using deprecated language features and generally about problematic code. As I pointed out, **zero-parameter K&R definitions are problematic even if we error out when they are called with the wrong number of parameters**, because such definitions might be inline parts of an interface that's used with other compilers that don't have such checks.

> If you do pursue this please use a distinct `-W` flag so it can be turned off without losing the benefits of `-Wstrict-prototypes`.

The warning is not on by default, and not enabled by `-Wall` or `-Wextra`, so this would serve the small sliver of developers who researched long enough to find this flag, decided to activate it only with Clang and not GCC, and then for some reason don't want it to do what it promises in its name. If the warning calls itself "strict", then that's what it should be.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66919