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

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Starting by looking at the test cases, I've got some suggestions on making the diagnostics a bit less confusing.



================
Comment at: clang/test/CodeGen/2009-06-01-addrofknr.c:8
 
-static int func(f)
+static int func(f) // expected-warning {{this function declaration without a prototype is deprecated in all versions of C and changes behavior in C2x}}
   void *f;
----------------
This message seems vaguer than necessary, since we know _for sure_ this is invalid.

Can we say something like: "K&R-style function definitions are deprecated in all versions of C and not supported in C2x"?


================
Comment at: clang/test/Parser/declarators.c:5
 
-void f0();
+void f0(); /* expected-warning {{a function declaration without a prototype is deprecated in all versions of C}} */
 void f1(int [*]);
----------------
Perhaps we should add an explanation to the message like
 `(specify '(void)' if you intended to accept no arguments)`
to make it clearer to folks who aren't familiar with this weirdness yet?


================
Comment at: clang/test/Sema/implicit-decl.c:25
+Boolean _CFCalendarDecomposeAbsoluteTimeV(const char *componentDesc, int32_t **vector, int32_t count) { // expected-error {{conflicting types}} \
+                                                                                                        // expected-warning {{this function declaration with a prototype changes behavior in C2x because it is preceded by a function without a prototype}}
  return 0;
----------------
It's not "preceded by a function without a prototype", though, it's preceeded by a implicit declaration due to a call. Which we would've already diagnosed with the default-on -Wimplicit-function-declaration diagnostic.

Although...what _is_ the desired behavior of an implicit function declaration in C2x mode? If we permit it _at all_, it must still create a non-prototype function declaration, I expect. In that case this decl with types would still be valid, so the warning is just wrong.



================
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}} \
----------------
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.


================
Comment at: clang/test/Sema/warn-deprecated-non-prototype.c:46
+                         strict-note {{this function declaration without a prototype changes behavior in C2x}}
+void order1(int i);   // both-warning {{this function declaration with a prototype changes behavior in C2x because it is preceded by a function without a prototype}}
+
----------------
Maybe something like "this function declaration will conflict with the preceding declaration in C2x, because the preceding declaration does not specify arguments."


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

https://reviews.llvm.org/D122895