aaron.ballman marked 3 inline comments as done.
aaron.ballman added inline comments.
================
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;
----------------
jyknight wrote:
> 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"?
I'd like to avoid using K&R in the diagnostic text (we're inconsistent about
K&R vs prototype, etc already, but I'd like to see us excise `K&R` from
diagnostics because it's not particularly descriptive to anyone younger than
about 40. :-D
How about: `function declarations without a prototype 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 [*]);
----------------
jyknight wrote:
> 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?
They're already offered a fixit for this situation, so I don't know that we
need the extra explanation? The user currently gets something like this:
```
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:16: warning:
declaration of a function without a prototype is deprecated in all versions of
C [-Wstrict-prototypes]
void other_func();
^
void
```
Do you still think the diagnostic needs to be made longer?
================
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;
----------------
jyknight wrote:
> 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.
>
> 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.
In order for that to work, Clang gins up an implicit declaration of `extern int
func();` which is the signature C89 requires, so I think the diagnostic here is
fine (it's also a very weird edge case I don't expect people to hit all that
often, since the only way to trigger this diagnostic is to define with a
prototype the implicitly-declared function).
There are no implicit function declarations after C89 (warned by default, but
should be an error). There's also no implicit int after C89 (not even warned on
by default, but should also be an error). Personally, I would like to see us
enable both diagnostics as warning-defaults-to-error in C99 through C17 (if
possible) and error only in C2x and forward, but I've not proposed it or worked
on it yet (fixing one horror show at a time, basically).
================
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}} \
----------------
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.
================
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}}
+
----------------
jyknight wrote:
> Maybe something like "this function declaration will conflict with the
> preceding declaration in C2x, because the preceding declaration does not
> specify arguments."
I can see the appeal, but that makes the diagnostic rather long and I'd still
like to be consistent in the terminology we use. Given that the old diagnostic
was `-Wstrict-prototypes` and the new one is `-Wdeprecated-non-prototype`, I
think we don't need to tie ourselves into knots to avoid using the terminology.
Do you feel strongly about rewording this?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122895/new/
https://reviews.llvm.org/D122895
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
