aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5529
+def warn_call_function_without_prototype : Warning<
+ "calling function %0 with arguments when function has no prototype">,
InGroup<
+ DiagGroup<"strict-calls-without-prototype">>, DefaultIgnore;
----------------
delcypher wrote:
> aaron.ballman wrote:
> > This diagnostic doesn't tell me what's wrong with the code (and in fact,
> > there's very possibly nothing wrong with the code whatsoever). Further, why
> > does calling a function *with no arguments* matter when it has no
> > prototype? I would imagine this should flag any call to a function without
> > a prototype given that the function without a prototype may still expect
> > arguments. e.g.,
> > ```
> > // Header.h
> > int f();
> >
> > // Source.c
> > int f(a) int a; { ... }
> >
> > // Caller.c
> > #include "Header.h"
> >
> > int main() {
> > return f();
> > }
> > ```
> > I think the diagnostic text should be updated to something more like
> > `cannot verify %0 is being called with the correct number or
> > %plural{1:type|:types}1 of arguments because it was declared without a
> > prototype` (or something along those lines that explains what's wrong with
> > the code).
> @aaron.ballman Thanks for the helpful feedback.
>
> > This diagnostic doesn't tell me what's wrong with the code (and in fact,
> > there's very possibly nothing wrong with the code whatsoever).
>
> That's a fair criticism. I think the diagnostic message you suggest is a lot
> more helpful so I'll go for something like that.
>
> > Further, why does calling a function *with no arguments* matter when it has
> > no prototype?
>
> The reason was to avoid the warning being noisy. E.g. I didn't the warning to
> fire in this situation.
>
> ```
> // Header.h
> int f(); // The user forgot to put `void` between parentheses
>
> // Source.c
> int f(void) { ... }
>
> // Caller.c
> #include "Header.h"
>
> int main() {
> return f();
> }
> ```
>
> Forgetting to put `void` in the declaration of `f()` is a pretty common thing
> to do because a lot of people read `int f()` as declaring a function that
> takes no arguments (it does in C++ but not in C).
>
> I don't want the warning to be noisy because I was planning on switching it
> on by default in open source and in a downstream use-case make it an error.
>
> How about this as a compromise? Split the warning into two separate warnings
>
> * `strict-call-without-prototype` - Warns on calls to functions without a
> prototype when no arguments are passed. Not enabled by default
> * `call-without-prototype` -Warns on calls to functions without a prototype
> when arguments are passed. Enable this by default.
>
> Alternatively we could enable both by default. That would still allow me to
> make `call-without-prototype` an error and `strict-call-without-prototype`
> not be an error for my downstream use-case.
>
> Thoughts?
> Forgetting to put void in the declaration of f() is a pretty common thing to
> do because a lot of people read int f() as declaring a function that takes no
> arguments (it does in C++ but not in C).
Yup, and this is exactly why I think we *should* be warning. That is a function
without a prototype, so the code is very brittle and dangerous at the call
site. The fact that the call site *currently* is correct doesn't mean it's
*intentionally* correct. e.g.,
```
// Header.h
int f(); // No prototype
// Source.c
int f(int a, int b) { return 0; } // Has a prototype, no diagnostic
// OtherSource.c
#include "Header.h"
int main() {
return f(); // No diagnostic with this patch, but still have the UB.
}
```
> I don't want the warning to be noisy because I was planning on switching it
> on by default in open source and in a downstream use-case make it an error.
Hmmm. Thinking out loud here.
Functions without prototypes were standardized in C89 as a deprecated feature
(C89 3.9.4, 3.9.5). I'd like to get to the point where any code that doesn't
pass `-ansi` is given a diagnostic (at least in pedantic mode outside of system
headers) about this deprecation, though I could probably be persuaded to keep
not diagnose in c89 mode if that's a massive pain point. But if in C99 or
later, I definitely see no reason not to diagnose the declarations as
deprecated by default.
However, calling a function without a prototype declaration is not itself
indicative of a programming mistake and is also not deprecated (it just stops
being a problem once all functions are required to have a prototype), so I'm
not certain it's well-motivated to enable the new diagnostic by default. This
is a bit more like use of VLAs, in that it's a common situation to accidentally
declare a function without a prototype. So having a "congrats, you're using
this feature" warning (like we did for `-Wvla`) for people who don't want to
accidentally use it seems reasonable. But "use" is a bit weird here -- this
flags call sites but the issue is with the declaration of the function, not
with its callers.
So I'm more of the opinion that we should be strengthening the diagnostics here
rather than weakening them, and I sort of think we should be focusing on the
declarations and not the call expressions. As a WG14 member for the community,
I'm definitely motivated to see us be more aggressive here because of proposals
like: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2841.htm. The committee
is trying to remove support for function declarations without prototypes, and
the empty paren case is basically the final sticking point. Diagnosing it more
appropriately to our users would help avoid nasty surprises.
Have you considered whether you could stomach strengthening
`-Wstrict-prototypes` by enabling it by default outside of `-ansi` (or perhaps
`-std=c89`)? I know this does not match GCC's behavior, but IIRC, GCC's
behavior came about because they implemented `-Wstrict-prototypes` in around
1990, aka, just as prototypes were being deprecated in C (so it would have been
incredibly disruptive to enable it at that point).
> Alternatively we could enable both by default. That would still allow me to
> make call-without-prototype an error and strict-call-without-prototype not be
> an error for my downstream use-case.
We could definitely split the diagnostic into two if we're convinced that
diagnosing call sites is the appropriate action to take.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116635/new/
https://reviews.llvm.org/D116635
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
