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

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.


================
Comment at: clang/test/Sema/knr-def-call.c:15
+void f2(float); // expected-note{{previous declaration is here}} \
+                   expected-warning {{this function declaration with a prototype changes behavior in C2x because it is followed by a function without a prototype}}
+void f2(x) float x; { } // expected-warning{{promoted type 'double' of K&R function parameter is not compatible with the parameter type 'float' declared in a previous prototype}} \
----------------
aaron.ballman wrote:
> jyknight wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > jyknight wrote:
> > > > > I think we don't want to emit a warning here. If a declaration with params doesn't match a K&R definition, we already emit a conflicting-types error.
> > > > > 
> > > > > [[Well...except for one case, I've noticed -- we don't current emit any warning when a definition with no parameters fails to match a preceding declaration with params, e.g. `void f(int); void f() { }`. I'm quite surprised -- I'm willing to say that such code is 100% just a bug, not intentional. I note that GCC unconditionally rejects it. I think we should also be emitting an unconditional error in that case.]]]
> > > > > 
> > > > > Anyhow, when you have a K&R style definition with parameters, that -- all by itself -- is definitely invalid in C2x, so we don't need to emit any warning on the declaration.
> > > > > I'm quite surprised -- I'm willing to say that such code is 100% just a bug, not intentional. I note that GCC unconditionally rejects it. I think we should also be emitting an unconditional error in that case.
> > > > 
> > > > I'd rather we be consistent here -- either every mixture of prototype/unprototyped is an error, or they're all a warning. I've added your example as a test case and we warn on it as being a change of behavior in C2x, which I think is defensible.
> > > > 
> > > > > Anyhow, when you have a K&R style definition with parameters, that -- all by itself -- is definitely invalid in C2x, so we don't need to emit any warning on the declaration.
> > > > 
> > > > I tend to agree, let me see what I can do.
> > > I addressed this so we no longer diagnose the function with a prototype in the case where it precedes the function without a prototype.
> > > I'd rather we be consistent here -- either every mixture of prototype/unprototyped is an error, or they're all a warning. I've added your example as a test case and we warn on it as being a change of behavior in C2x, which I think is defensible.
> > 
> > Even before, we are NOT consistent. We emit an error on `void f(int); void f(x) float x; {}`, but not for `void f(int); void f() {}`. In both cases, we have a prototyped declaration, followed by an old-style "prototypeless" definition. I think it would be sensible to diagnose with an unconditional error in both cases, not only the former.
> > Even before, we are NOT consistent. We emit an error on void f(int); void f(x) float x; {}, but not for void f(int); void f() {}. In both cases, we have a prototyped declaration, followed by an old-style "prototypeless" definition. I think it would be sensible to diagnose with an unconditional error in both cases, not only the former.
> 
> I don't think `void f(int); void f() {}` should be diagnosed solely as an issue with strict prototypes; I think it should be diagnosed the same as `void f(int); void f(x) float x; {}`, which is *not* about strict prototypes but *is* about the fact that the function types cannot merge because they conflict. Once there's a declaration with a prototype, the function always has a prototype (see C17 6.2.7p3 ) and redeclarations (including for definition) need to have a compatible signature (see C17 6.7.6.3p15). So I'd expect a `conflicting types` error diagnostic. I think it's defensible to *additionally* issue the strict prototypes warning (though if we can silence the warning because we're already issuing an error without introducing significant extra complexity, that would be ideal). I'll look into adding the error diagnostic.
Sorry about being unclear -- what you propose is precisely what I meant to say.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895