[PATCH] D114787: [clang][PR51931] Enable `-Wdeclaration-after-statement` for all C versions

[Aaron Ballman via Phabricator via cfe-commits]

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