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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
