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

[zero9178]

Summary

-Wdeclaration-after-statement currently only outputs an diagnostic if the user is compiling in C versions older than C99, even if the warning was explicitly requested by the user.
This patch makes the warning also available in later C versions. If the C version is C99 or later it is simply a normal warning that is disabled by default (as it is valid C99) and has to be enabled by users. In older versions it remains an extension warning, and therefore affected by -pedantic.

The above behaviour also matches GCCs behaviour.

Fixes https://bugs.llvm.org/show_bug.cgi?id=51931

[aaron.ballman]

https://reviews.llvm.org/D115094 is a review for doing effectively the same fix. Can you coordinate with the other patch author to determine who will drive this fix?

[zero9178]

In D114787#3188735, @aaron.ballman wrote:

https://reviews.llvm.org/D115094 is a review for doing effectively the same fix. Can you coordinate with the other patch author to determine who will drive this fix?

I sent an email to the author on December 13th but have yet to hear back from them. Should we move forward with this version of the patch then?

[aaron.ballman]

In D114787#3218382, @zero9178 wrote:

In D114787#3188735, @aaron.ballman wrote:

https://reviews.llvm.org/D115094 is a review for doing effectively the same fix. Can you coordinate with the other patch author to determine who will drive this fix?

I sent an email to the author on December 13th but have yet to hear back from them. Should we move forward with this version of the patch then?

I'd like to give a little bit more time (and another ping) only because of the recent holiday season, but I think we don't need to wait too much longer. I'm still digging out from under my pile of reviews, so I'd not be surprised if others are in a similar boat.

How about we proceed with this patch if there's no response by Mon Jan 10?

In the meantime, I have some minor feedback.

clang/test/Sema/warn-mixed-decls.c

2–5

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

[zero9178]

Addressed reviewer comments: Added tests for cases when the diagnostic should not be emitted.

[zero9178]

clang/test/Sema/warn-mixed-decls.c

2–5

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.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

9865

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

clang/test/Sema/warn-mixed-decls.c

2–5

Huh, today I learned. :-D Thanks for letting me know!

[zero9178]

Improved error messages and adjusted tests accordingly

[zero9178]

clang/include/clang/Basic/DiagnosticSemaKinds.td

9865

A CPre99Compat diagnostic group sounds like a good idea if more such warnings would be requested by users.

[aaron.ballman]

LGTM! Thanks for the improvement!

clang/include/clang/Basic/DiagnosticGroups.td

def DocumentationPedantic : DiagGroup<"documentation-pedantic",
                                       [DocumentationUnknownCommand]>;
 def DocumentationDeprecatedSync : DiagGroup<"documentation-deprecated-sync">;
 def Documentation : DiagGroup<"documentation",
                               [DocumentationHTML,
                                DocumentationDeprecatedSync]>;
 
 def EmptyBody : DiagGroup<"empty-body">;
 def Exceptions : DiagGroup<"exceptions">;
+def DeclarationAfterStatement : DiagGroup<"declaration-after-statement">;
 
 def GNUEmptyInitializer : DiagGroup<"gnu-empty-initializer">;
 def GNUEmptyStruct : DiagGroup<"gnu-empty-struct">;
 def ExtraTokens : DiagGroup<"extra-tokens">;
 def CXX98CompatExtraSemi : DiagGroup<"c++98-compat-extra-semi">;
 def CXX11ExtraSemi : DiagGroup<"c++11-extra-semi">;
 def EmptyInitStatement : DiagGroup<"empty-init-stmt">;
 def ExportUnnamed : DiagGroup<"export-unnamed">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_builtin_longjmp_invalid_val : Error<
   "argument to __builtin_longjmp must be a constant 1">;
 def err_builtin_requires_language : Error<"'%0' is only available in %1">;
 
 def err_constant_integer_arg_type : Error<
   "argument to %0 must be a constant integer">;
 
 def ext_mixed_decls_code : Extension<
   "ISO C90 forbids mixing declarations and code">,
-  InGroup<DiagGroup<"declaration-after-statement">>;
+  InGroup<DeclarationAfterStatement>;
+def warn_mixed_decls_code : Warning<
+  ext_mixed_decls_code.Text>,
+  InGroup<DeclarationAfterStatement>, DefaultIgnore;
 
 def err_non_local_variable_decl_in_for : Error<
   "declaration of non-local variable in 'for' loop">;
 def err_non_variable_decl_in_for : Error<
   "non-variable declaration in 'for' loop">;
 def err_toomany_element_decls : Error<
   "only one element declaration is allowed">;
 def err_selector_element_not_lvalue : Error<

clang/lib/Sema/SemaStmt.cpp

 sema::CompoundScopeInfo &Sema::getCurCompoundScope() const {
   return getCurFunction()->CompoundScopes.back();
 }
 
 StmtResult Sema::ActOnCompoundStmt(SourceLocation L, SourceLocation R,
                                    ArrayRef<Stmt *> Elts, bool isStmtExpr) {
   const unsigned NumElts = Elts.size();
 
-  // If we're in C89 mode, check that we don't have any decls after stmts.  If
-  // so, emit an extension diagnostic.
-  if (!getLangOpts().C99 && !getLangOpts().CPlusPlus) {
+  // If we're in C mode, check that we don't have any decls after stmts.  If
+  // so, emit an extension diagnostic in C89 and potentially a warning in later
+  // versions.
+  if (!getLangOpts().CPlusPlus) {
     // Note that __extension__ can be around a decl.
     unsigned i = 0;
     // Skip over all declarations.
     for (; i != NumElts && isa<DeclStmt>(Elts[i]); ++i)
       /*empty*/;
 
     // We found the end of the list or a statement.  Scan for another declstmt.
     for (; i != NumElts && !isa<DeclStmt>(Elts[i]); ++i)
       /*empty*/;
 
     if (i != NumElts) {
       Decl *D = *cast<DeclStmt>(Elts[i])->decl_begin();
-      Diag(D->getLocation(), diag::ext_mixed_decls_code);
+      Diag(D->getLocation(), !getLangOpts().C99 ? diag::ext_mixed_decls_code
+                                                : diag::warn_mixed_decls_code);
     }
   }
 
   // Check for suspicious empty body (null statement) in `for' and `while'
   // statements.  Don't do anything for template instantiations, this just adds
   // noise.
   if (NumElts != 0 && !CurrentInstantiationScope &&
       getCurCompoundScope().HasEmptyLoopBodies) {

clang/test/Sema/warn-mixed-decls.c

+/* RUN: %clang_cc1 -fsyntax-only -verify -std=c89 -pedantic %s
+ */
+/* RUN: %clang_cc1 -fsyntax-only -verify -std=c99 -Wdeclaration-after-statement %s
+ */
+
+/* Should not emit diagnostic when not pedantic, not enabled or in C++ Code*/
+/* RUN: %clang_cc1 -fsyntax-only -verify=none -std=c89 %s
+ */
+/* 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 */
+
+int foo(int i)
+{
+  i += 1;
+  int f = i; /* expected-warning {{ISO C90 forbids mixing declarations and code}}*/
+  return f;
+}