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:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > 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` ?
> > I went with `a function declaration without a prototype is deprecated in
> > all versions of C and is not supported in C2x`, let me know if you think it
> > needs further adjusting. I also reworded the other diagnostics to sound
> > similar.
> I do agree with you that "K&R" is perhaps not the best name either...
>
> But I think this warning message may still be confusing to users. A typical C
> user I think will be surprised to see a warning about a "declaration"
> pointing to a definition -- even though that is technically accurate.
> Especially when we're complaining concretely about the weird old definition
> syntax, it would be better to say "definition".
>
> It looks like the most common name for this is "old-style function
> definition" -- both GCC and MSVC use that name, and clang also did, before.
> (Hmmm. And GCC actually seems to place this particular warning under the
> off-by-default -Wold-style-definition flag -- which clang doesn't implement.)
> I don't know that "old-style" is a great description either, but "old-style
> function definition" definitely seems more understandable than "function
> declaration without a prototype" when talking about a K&R-style
> separated-arguments-types definition.
> But I think this warning message may still be confusing to users. A typical C
> user I think will be surprised to see a warning about a "declaration"
> pointing to a definition -- even though that is technically accurate.
> Especially when we're complaining concretely about the weird old definition
> syntax, it would be better to say "definition".
I can look into making it say "definition" when written on a definition, but
this feels like it's starting to split awfully fine hairs to me, so if it turns
out to add more code than I think it's worth, I may stick with "declaration"
because it's still correct and I don't think the confusion should impede a
user's ability to fix the bug *too much*.
================
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:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > 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?
> > I elected to leave this alone unless you feel strongly.
> I guess I sort of wondered if users might consider "without a prototype" to
> generally indicate "implicit declaration". So, the wording "declaration
> without a prototype" might be like..."wait you're complaining that my
> declaration doesn't have a declaration? Huh? It's right there!"
>
> But, with the fixit hint suggesting the addition of "void", that's probably
> sufficiently explanatory.
>
> I guess I sort of wondered if users might consider "without a prototype" to
> generally indicate "implicit declaration". So, the wording "declaration
> without a prototype" might be like..."wait you're complaining that my
> declaration doesn't have a declaration? Huh? It's right there!"
That's a good point, we should watch to see if that sort of confusion does
happen in practice (I'll keep my eyes peeled), but I also think the fix-it
should be sufficiently clarifying for most folks.
================
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:
> 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.
Repository:
rG LLVM Github Monorepo
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
