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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to