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:
> > 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.
> 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.
To me, it's about what action the user can take to respond to the diagnostic. I
don't see the complement adding value in that regard. The user is calling a
function that's declared somewhere without a prototype, but unless the user has
control over the declaration, there is *nothing* they can do at the call site
about it. Well, almost. They could add their own redeclaration with a
prototype, but that's actually *worse* because now the redeclaration might get
out of sync with the actual definition, but the users won't get any warnings
about it.
Basically, I don't think it's a problem worth diagnosing to *call* a function
without a prototype, but I definitely agree it's more dangerous to *have*
functions without prototypes which are called. So I'm absolutely sold on
warning users about the dangers here, but I don't think a new warning is the
right approach when we could strengthen the existing warning. The current
diagnostic is effectively `-Wstrict-prototypes-but-only-when-called`.
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
