aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9865 def ext_mixed_decls_code : Extension< "ISO C90 forbids mixing declarations and code">, + InGroup<DeclarationAfterStatement>; ---------------- In the other review, I left a comment about the diagnostic text: https://reviews.llvm.org/D115094#3176985 Since we're cleaning this up, I think we should reword this diagnostic so it follows newer conventions. I think the extension diagnostic should read: `mixing declarations and code is a C99 extension` and the default ignore warning should read `mixing declarations and code is incompatible with standards before C99`. (This also helpfully removes the `ISO C90` wording, which is confused about the name of the standard.) Typically, we'd put the default ignore warning under a new `CPre99Compat` diagnostic group (spelled `pre-c99-compat`) as we do with other precompat diagnostics, but the goal here is to match GCC's behavior and so the existing warning group seems fine to me (I don't think we want warnings in multiple groups, but that's possibly an option if it matters in the future). ================ Comment at: clang/test/Sema/warn-mixed-decls.c:1-4 +/* RUN: %clang_cc1 -fsyntax-only -verify -std=c89 -pedantic %s + */ +/* RUN: %clang_cc1 -fsyntax-only -verify -std=c99 -Wdeclaration-after-statement %s + */ ---------------- zero9178 wrote: > aaron.ballman wrote: > > I'd also like to see RUN lines for when we expect the diagnostic to not be > > enabled: > > ``` > > /* RUN: %clang_cc1 -fsyntax-only -verify=none -std=c99 %s */ > > /* RUN: %clang_cc1 -fsyntax-only -verify=none -x c++ %s */ > > /* RUN: %clang_cc1 -fsyntax-only -verify=none -x c++ > > -Wdeclaration-after-statement %s */ > > > > /* none-no-diagnostics */ > > ``` > > I should note that the last RUN line will give different behavior between > > Clang and GCC: https://godbolt.org/z/o1PKo7dhM, but I think that's a more > > general issue that doesn't need to be addressed in this patch. (We don't > > have a way to flag a diagnostic as requiring a particular language mode.) > The `*/` on the next line seems to be necessary so as lit seems to otherwise > add `*/` to the command line of the `RUN` command. At least this is the case > on my Windows machine. Huh, today I learned. :-D Thanks for letting me know! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114787/new/ https://reviews.llvm.org/D114787 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
