delcypher 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;
----------------
aaron.ballman wrote:
> 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.
> 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
True. It's not necessarily a mistake but calling functions without a prototype
is very error prone due the lack of argument count and type checking. This is
why I think it might be worth flagging the potential problem by default. I'm
happy to not have it on by default if that is the general consensus.
> 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.
You're right that the underlying issue is at the declaration and not at the
call sites. However, if I wanted to warn about all the declarations I would
just use `-Wstrict-prototypes` which is already implemented in Clang. I don't
consider that warning very pragmatic because it'll warn about functions that
I'm not calling which could make it extremely noisy. Instead I wanted a more
pragmatic (less noisy) warning. I consider what I'm proposing to be more
pragmatic because if a function is missing a prototype, it **only matters when
it is called** (i.e. this is where the lack of a prototype will cause problems
if the arguments/types aren't "just right"). If I'm not calling a function I
don't want to be told its missing a prototype because it does not matter for
the current compilation unit.
> Have you considered whether you could stomach strengthening
> -Wstrict-prototypes by enabling it by default outside of -ansi (or perhaps
> -std=c89)?
As I said above I don't think ` -Wstrict-prototypes` is very pragmatic. It's
probably good if you're trying to audit a header file, but noisy if you're
actually trying to compile code.
Thinking about it I guess what I've proposed is a complement to
`-Wstrict-prototypes`. I don't see why clang couldn't have both warnings.
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
