[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5566-5567
+def warn_non_prototype_changes_behavior : Warning<
+  "this function declaration without a prototype is deprecated in all versions "
+  "of C and changes behavior in C2x">, InGroup<DeprecatedNonPrototype>;
+def warn_prototype_changes_behavior : Warning<
----------------
cor3ntin wrote:
> "declaration of a function without a prototype is deprecated", may be slightly better?
Sure, I can reword the rest so they're consistent.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:3881
+      if (WithProto->getNumParams() != 0) {
+        // The function definition has parameters, so this will change
+        // behavior in C2x.
----------------
cor3ntin wrote:
> I wonder if
> 
>   - This shouldn't be placed in a separate function for clarity
>   - We  could bail out early for built ins ?
> 
> 
The change looks more complicated than it is because of the indentation changes -- I'm not certain putting this in its own function gains a whole lot (it's not reusable below; I already looked into doing that and wasn't happy with the results).

We can't bail out early for builtins because we still issue diagnostics in cases where only one side is a builtin. e.g.,
```
void exit(); // We still want to diagnose this as changing behavior specifically
```
See test/Sema/knr-variadic-def.c for this test case.



================
Comment at: clang/lib/Sema/SemaType.cpp:5560-5562
   if (!LangOpts.CPlusPlus &&
-      D.getFunctionDefinitionKind() == FunctionDefinitionKind::Declaration) {
+      (D.getFunctionDefinitionKind() == FunctionDefinitionKind::Declaration ||
+       D.getFunctionDefinitionKind() == FunctionDefinitionKind::Definition)) {
----------------
cor3ntin wrote:
> `if (!LangOpts.CPlusPlus)` is probably enough here
Hmm, yeah, you're right. Good catch!


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

https://reviews.llvm.org/D122895