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

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Looks reasonable overall.

I've added a few specific comments to some tests, but in general, I think we should avoid adding -std=c99 to test-cases to workaround implicit decl issues, unless:

- the test is explicitly testing the behavior of implicit decls (potentially in interaction with some other feature).
- it's a regression test, with some particular reduced/artificial inputs.
- the ARM codegen tests, for now, on the assumption the arm maintainers will address it separately.



================
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.
----------------
This is just a typo, you won't need the -std=c99 on this test if you fix pointeeConverison -> pointeeConversion


================
Comment at: clang/docs/ReleaseNotes.rst:141-146
+- The ``-Wimplicit-function-declaration`` warning diagnostic now defaults to
+  emitting an error (which can be downgraded to a warning with
+  ``-Wno-error=implicit-function-declaration`` or disabled entirely with
+  ``-Wno-implicit-function-declaration``) in C11 and
+  C17 mode. This is because the feature was removed in C99 and cannot be
+  supported in C2x.
----------------
Some wording suggestions here, to remove parenthetical, and more explicitly note that the options have no effect in C2x.


================
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;
----------------
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).


================
Comment at: clang/test/CodeGen/PowerPC/builtins-ppc-p8vector.c:4
 // RUN: %clang_cc1 -target-feature +altivec -target-feature +power8-vector -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck %s -check-prefix=CHECK-LE
-// RUN: not %clang_cc1 -target-feature +altivec -target-feature +vsx -triple powerpc64-unknown-unknown -emit-llvm %s -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PPC
+// RUN: not %clang_cc1 -std=c99 -target-feature +altivec -target-feature +vsx -triple powerpc64-unknown-unknown -emit-llvm %s -o - 2>&1 | FileCheck %s -check-prefix=CHECK-PPC
 // Added -target-feature +vsx above to avoid errors about "vector double" and to
----------------
This test-run is already expected to fail, so the -std=c99 seems like it shouldn't be necessary. I think just change the CHECK-PPC lines looking for "warning: implicit declaration" to look for "error: implicit declaration" instead.


================
Comment at: clang/test/CodeGen/X86/bmi2-builtins.c:1
-// RUN: %clang_cc1 -ffreestanding %s -triple=x86_64-apple-darwin -target-feature +bmi2 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -ffreestanding %s -triple=x86_64-apple-darwin -target-feature +bmi2 -emit-llvm -std=c99 -o - | FileCheck %s
 // RUN: %clang_cc1 -ffreestanding %s -triple=i386-apple-darwin -target-feature +bmi2 -emit-llvm -o - | FileCheck %s --check-prefix=B32
----------------
That this is needed looks like a test bug here -- it should be testing _mulx_u64 on x86-64 and _mulx_u32 on i386.


================
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.
----------------
Declare `int bar(struct s*, struct s*);` instead


================
Comment at: clang/test/Headers/arm-cmse-header-ns.c:1
-// RUN:     %clang_cc1 -triple thumbv8m.base-eabi -fsyntax-only        %s 2>&1 | FileCheck --check-prefix=CHECK-c %s
+// RUN:     %clang_cc1 -triple thumbv8m.base-eabi -fsyntax-only -std=c99 %s 2>&1 | FileCheck --check-prefix=CHECK-c %s
 // RUN: not %clang_cc1 -triple thumbv8m.base-eabi -fsyntax-only -x c++ %s 2>&1 | FileCheck --check-prefix=CHECK-cpp %s
----------------
Maybe better to edit the CHECK-c lines to expect an error?


================
Comment at: clang/test/Modules/modulemap-locations.m:2
 // RUN: rm -rf %t 
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/ModuleMapLocations/Module_ModuleMap -I %S/Inputs/ModuleMapLocations/Both -F %S/Inputs/ModuleMapLocations -I %S/Inputs/ModuleMapLocations -F %S/Inputs -x objective-c -fsyntax-only %s -verify -Wno-private-module
+// RUN: %clang_cc1 -std=c99 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/ModuleMapLocations/Module_ModuleMap -I %S/Inputs/ModuleMapLocations/Both -F %S/Inputs/ModuleMapLocations -I %S/Inputs/ModuleMapLocations -F %S/Inputs -x objective-c -fsyntax-only %s -verify -Wno-private-module
 
----------------
Change expected-warnings to expected-error instead of -std=c99?


================
Comment at: clang/test/PCH/chain-macro-override.c:13
   h();
   h2(); // expected-warning{{implicit declaration of function 'h2' is invalid in C99}}
   h3();
----------------
Change to expected-error and delete -std=c99?


================
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 '('}} \
----------------
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?).


================
Comment at: clang/test/Sema/implicit-ms-builtin-decl.c:41
 void h(void) {
   (void)__mulh(21LL, 2LL);  // expected-warning{{implicit declaration of function '__mulh' is invalid}}
   (void)__umulh(21ULL, 2ULL);  // expected-warning{{implicit declaration of function '__umulh' is invalid}}
----------------
switch to expected-error and remove -std=c99?


================
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
 
----------------
I can't quite follow what this test is doing, but it looks like it does intend to have a prototype available?


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

https://reviews.llvm.org/D122983