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