[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 10 inline comments as done.
aaron.ballman added a comment.

Thank you for the thorough double-check on the test changes, @jyknight! I was able to remove most of the ones you called out (I commented where I couldn't) plus a few more `-std=c99` from RUN lines, but there are quite a few left because it's entirely unclear what the test is actually testing (especially in CodeGen tests -- we have these RUN lines but they don't check anything, so I'm assuming these are "don't crash" tests reduced from user code, so I left the -std=c99 for them).



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c:76
 
 void pointeeConverison(int *IP, double *DP) { pointeeConversion(DP, IP); }
 // NO-WARN: Even though this is possible in C, a swap is diagnosed by the compiler.
----------------
jyknight wrote:
> This is just a typo, you won't need the -std=c99 on this test if you fix pointeeConverison -> pointeeConversion
Good catch, I'll fix this up in an NFC change.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:423-426
+def ext_implicit_function_decl_c11 : ExtWarn<
+  "implicit declaration of function %0 is invalid in C99 and later and "
+  "unsupported in C2x">,
+  InGroup<ImplicitFunctionDeclare>, DefaultError;
----------------
jyknight wrote:
> I think it'd be good to use the same message for both variants. (Even if you're building in c99, might as well note it's gone in C2x).
Heh, I waffled on doing exactly that, so I'm glad for this feedback!


================
Comment at: clang/test/CodeGen/misaligned-param.c:1
-// RUN: %clang_cc1 %s -triple i386-apple-darwin -emit-llvm -o - | FileCheck %s
 // Misaligned parameter must be memcpy'd to correctly aligned temporary.
----------------
jyknight wrote:
> Declare `int bar(struct s*, struct s*);` instead
Good catch; I think I was worried this was intending to test the function call, which is why I added the `-std=c89`.


================
Comment at: clang/test/Sema/implicit-decl.c:30
+  __builtin_is_les(1, 3); // expected-error{{use of unknown builtin '__builtin_is_les'}} \
+                             c2x-error {{unknown type name '__builtin_is_les'; did you mean '__builtin_va_list'?}} \
+                             c2x-error {{expected identifier or '('}} \
----------------
jyknight wrote:
> Yikes! That's a particularly awful typo-fix suggestion. I assume from those messages, it decided this should be a function declaration instead of a call, and is parsing as a function declaration?
> 
> I'd say that's unrelated to this change, EXCEPT, that doesn't happen in C++ mode. So I'm guessing there's some weird interaction here that triggers ONLY in the C mode of the parser, which causes it to prefer to parse unknown identifiers as a type name instead of a variable/function name (on the assumption that implicit function decls exist, and would've triggered already, if relevant, perhaps?).
> Yikes! That's a particularly awful typo-fix suggestion. I assume from those messages, it decided this should be a function declaration instead of a call, and is parsing as a function declaration?

That's correct.

> I'd say that's unrelated to this change, EXCEPT, that doesn't happen in C++ mode. So I'm guessing there's some weird interaction here that triggers ONLY in the C mode of the parser, which causes it to prefer to parse unknown identifiers as a type name instead of a variable/function name (on the assumption that implicit function decls exist, and would've triggered already, if relevant, perhaps?).

That seems to be correct. It appears we get into this state because of implicit int. So I'd like to leave this alone for now and try to revisit later when I try to treat implicit int similar to what I've done in this patch for implicit function declarations.


================
Comment at: clang/test/VFS/module_missing_vfs.m:2
 // RUN: rm -rf %t && mkdir -p %t
 // RUN: echo "void funcA(void);" >> %t/a.h
 
----------------
jyknight wrote:
> I can't quite follow what this test is doing, but it looks like it does intend to have a prototype available?
Yeah, I struggled mightily to figure out what this test was doing and eventually gave up. I *think* the behavior of the test is that it's putting the declaration in a.h and then overlaying with an a.h (in Inputs/MissingVFS) that does not have the declaration, and demonstrating that you can still call it? But I honestly don't know.


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

https://reviews.llvm.org/D122983