[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Dan Liew via Phabricator via cfe-commits]

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:
> 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116635/new/

https://reviews.llvm.org/D116635