[clang] support Wold-style-declaration #78837

SihangZhu · 2024-01-20T09:25:17Z

Storage-class specifiers like static are not the first things in a declaration. According to the C Standard, this usage is obsolescent. This patch add a diagnose to warn it.
This is a counterpart of gcc's Wold-style-declaration.

llvmbot · 2024-01-20T09:25:47Z

@llvm/pr-subscribers-clang

Author: None (SihangZhu)

Changes

Storage-class specifiers like static are not the first things in a declaration. According to the C Standard, this usage is obsolescent. This patch add a diagnose to warn it.
This is a counterpart of gcc's Wold-style-declaration.

Full diff: https://github.com/llvm/llvm-project/pull/78837.diff

4 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticCommonKinds.td (+2)
(modified) clang/include/clang/Basic/DiagnosticGroups.td (+2)
(modified) clang/lib/Parse/ParseDecl.cpp (+11)
(added) clang/test/Parser/old-style-declaration.c (+33)

diff --git a/clang/include/clang/Basic/DiagnosticCommonKinds.td b/clang/include/clang/Basic/DiagnosticCommonKinds.td
index 5544dc88004d9a..0d29316767e950 100644
--- a/clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ b/clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -84,6 +84,8 @@ def err_param_redefinition : Error<"redefinition of parameter %0">;
 def warn_method_param_redefinition : Warning<"redefinition of method parameter %0">;
 def warn_method_param_declaration : Warning<"redeclaration of method parameter %0">,
   InGroup<DuplicateArgDecl>, DefaultIgnore;
+def warn_old_style_declaration: Warning <"'%0' is not at beginning of declaration">,
+  InGroup<OldStyleDeclaration>, DefaultIgnore;
 def err_invalid_storage_class_in_func_decl : Error<
   "invalid storage class specifier in function declarator">;
 def err_expected_namespace_name : Error<"expected namespace name">;
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 6765721ae7002c..ebe751a9947971 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -578,6 +578,7 @@ def ObjCPointerIntrospect : DiagGroup<"deprecated-objc-pointer-introspection", [
 def ObjCMultipleMethodNames : DiagGroup<"objc-multiple-method-names">;
 def ObjCFlexibleArray : DiagGroup<"objc-flexible-array">;
 def ObjCBoxing : DiagGroup<"objc-boxing">;
+def OldStyleDeclaration : DiagGroup<"old-style-declaration">;
 def CompletionHandler : DiagGroup<"completion-handler">;
 def CalledOnceParameter : DiagGroup<"called-once-parameter", [CompletionHandler]>;
 def OpenCLUnsupportedRGBA: DiagGroup<"opencl-unsupported-rgba">;
@@ -1026,6 +1027,7 @@ def Extra : DiagGroup<"extra", [
     EmptyInitStatement,
     StringConcatation,
     FUseLdPath,
+    OldStyleDeclaration,
   ]>;
 
 def Most : DiagGroup<"most", [
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 356e7851ec639c..ab1555ebc19088 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -3348,6 +3348,7 @@ void Parser::ParseDeclarationSpecifiers(
   while (true) {
     bool isInvalid = false;
     bool isStorageClass = false;
+    bool isFunctionSpecifier = false;
     const char *PrevSpec = nullptr;
     unsigned DiagID = 0;
 
@@ -4092,6 +4093,7 @@ void Parser::ParseDeclarationSpecifiers(
     // function-specifier
     case tok::kw_inline:
       isInvalid = DS.setFunctionSpecInline(Loc, PrevSpec, DiagID);
+      isFunctionSpecifier = true;
       break;
     case tok::kw_virtual:
       // C++ for OpenCL does not allow virtual function qualifier, to avoid
@@ -4104,6 +4106,7 @@ void Parser::ParseDeclarationSpecifiers(
         isInvalid = true;
       } else {
         isInvalid = DS.setFunctionSpecVirtual(Loc, PrevSpec, DiagID);
+        isFunctionSpecifier = true;
       }
       break;
     case tok::kw_explicit: {
@@ -4140,12 +4143,14 @@ void Parser::ParseDeclarationSpecifiers(
       }
       isInvalid = DS.setFunctionSpecExplicit(ExplicitLoc, PrevSpec, DiagID,
                                              ExplicitSpec, CloseParenLoc);
+      isFunctionSpecifier = true;
       break;
     }
     case tok::kw__Noreturn:
       if (!getLangOpts().C11)
         Diag(Tok, diag::ext_c11_feature) << Tok.getName();
       isInvalid = DS.setFunctionSpecNoreturn(Loc, PrevSpec, DiagID);
+      isFunctionSpecifier = true;
       break;
 
     // alignment-specifier
@@ -4552,6 +4557,12 @@ void Parser::ParseDeclarationSpecifiers(
       continue;
     }
 
+    unsigned Specs = DS.getParsedSpecifiers();
+    if (!getLangOpts().CPlusPlus && (isFunctionSpecifier || isStorageClass)) {
+      if (Specs & DeclSpec::PQ_TypeQualifier || DS.hasTypeSpecifier())
+        Diag(Tok, diag::warn_old_style_declaration) << Tok.getName();
+    }
+
     DS.SetRangeEnd(ConsumedEnd.isValid() ? ConsumedEnd : Tok.getLocation());
 
     // If the specifier wasn't legal, issue a diagnostic.
diff --git a/clang/test/Parser/old-style-declaration.c b/clang/test/Parser/old-style-declaration.c
new file mode 100644
index 00000000000000..e7ccea32bfa957
--- /dev/null
+++ b/clang/test/Parser/old-style-declaration.c
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wold-style-declaration %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wextra %s
+
+static int x0;
+int __attribute__ ((aligned (16))) static x1; // expected-warning {{'static' is not at beginning of declaration}}
+
+extern int x2;
+int extern x3; // expected-warning {{'extern' is not at beginning of declaration}}
+
+typedef int x4;
+int typedef x5; // expected-warning {{'typedef' is not at beginning of declaration}}
+
+void g (int);
+
+void
+f (void)
+{
+  auto int x6 = 0;
+  int auto x7 = 0; // expected-warning {{'auto' is not at beginning of declaration}}
+  register int x8 = 0;
+  int register x9 = 0; // expected-warning {{'register' is not at beginning of declaration}}
+  g (x6 + x7 + x8 + x9);
+}
+
+const static int x10; // expected-warning {{'static' is not at beginning of declaration}}
+
+/* Attributes are OK before storage class specifiers, since some
+   attributes are like such specifiers themselves.  */
+
+__attribute__((format(printf, 1, 2))) static void h (const char *, ...);
+__attribute__((format(printf, 1, 2))) void static i (const char *, ...); // expected-warning {{'static' is not at beginning of declaration}}
+
+static __thread int var = 5; // not-expected-warning {{'__thread' is not at beginning of declaration}}
\ No newline at end of file

SihangZhu · 2024-01-24T07:31:20Z

ping

SihangZhu · 2024-02-19T11:14:12Z

ping

repeat:)

AaronBallman

Thank you for working on this! Please be sure to add release notes to clang/docs/ReleaseNotes.rst so users know about the new diagnostic capabilities.

AaronBallman · 2024-02-20T19:46:49Z

clang/include/clang/Basic/DiagnosticCommonKinds.td

+def warn_old_style_declaration: Warning <"'%0' is not at beginning of declaration">,
+  InGroup<OldStyleDeclaration>, DefaultIgnore;


We don't typically add new diagnostics that are off by default (though this one is in -Wextra so it has a chance of being enabled by users). I actually wonder if we want to enable this by default; the standard makes this an obsolescent feature (see C23 6.11.5p1) and so having an on-by-default warning actually helps alert users to potential future code breakage.

My intuition is that this would not be an overly chatty diagnostic to enable by default; do you have access to a large corpus of C code you could try to compile with your patch?

AaronBallman · 2024-02-20T19:49:56Z

clang/lib/Parse/ParseDecl.cpp

@@ -4104,6 +4106,7 @@ void Parser::ParseDeclarationSpecifiers(
        isInvalid = true;
      } else {
        isInvalid = DS.setFunctionSpecVirtual(Loc, PrevSpec, DiagID);
+        isFunctionSpecifier = true;


C doesn't have virtual functions, so no need for this change.

AaronBallman · 2024-02-20T19:50:14Z

clang/lib/Parse/ParseDecl.cpp

@@ -4140,12 +4143,14 @@ void Parser::ParseDeclarationSpecifiers(
      }
      isInvalid = DS.setFunctionSpecExplicit(ExplicitLoc, PrevSpec, DiagID,
                                             ExplicitSpec, CloseParenLoc);
+      isFunctionSpecifier = true;


C also doesn't have explicit.

AaronBallman · 2024-02-20T19:55:47Z

clang/lib/Parse/ParseDecl.cpp

@@ -3348,6 +3348,7 @@ void Parser::ParseDeclarationSpecifiers(
  while (true) {
    bool isInvalid = false;
    bool isStorageClass = false;
+    bool isFunctionSpecifier = false;


Hmmm, what's obsoleted are storage class specifiers that are not at the start of the declaration. e.g.,

int static i = 12;

and function specifiers are not storage class specifiers. So technically, this is not obsolete:

void _Noreturn func(void);

GCC does warn about this, and I think that's reasonable to also mimic. I'll check with WG14 whether function specifiers should also be obsoleted similar to storage class specifiers.

AaronBallman · 2024-02-20T19:57:16Z

clang/test/Parser/old-style-declaration.c

+  auto int x6 = 0;
+  int auto x7 = 0; // expected-warning {{'auto' is not at beginning of declaration}}


This is going to cause problems when we update to -std=c23, so it might be good to guard this on __STDC_VERSION__ to save ourselves some headaches later.

AaronBallman · 2024-02-20T20:00:27Z

clang/test/Parser/old-style-declaration.c

+__attribute__((format(printf, 1, 2))) static void h (const char *, ...);
+__attribute__((format(printf, 1, 2))) void static i (const char *, ...); // expected-warning {{'static' is not at beginning of declaration}}
+
+static __thread int var = 5; // not-expected-warning {{'__thread' is not at beginning of declaration}}


You should add tests for: _Noreturn, inline, _Thread_local, constexpr (C23), auto (C23 usage, not C89 usage).

I think it also makes sense to add a test for alignas but I think we should allow that in any position under the same logic for allowing attributes.

Also, please add a newline to the end of the file.

AaronBallman · 2024-02-20T20:02:49Z

clang/test/Parser/old-style-declaration.c

+// RUN: %clang_cc1 -fsyntax-only -verify -Wextra %s
+


It would be good to add a RUN line along these lines:

Suggested change

// RUN: %clang_cc1 -fsyntax-only -verify -Wextra %s

// RUN: %clang_cc1 -fsyntax-only -verify -Wextra %s

// RUN: %clang_cc1 -fsyntax-only -verify=cpp -Wextra -x c++ %s

// cpp-no-diagnostics

and group the code that's invalid in C++ under #ifndef __cplusplus, to show that the diagnostics are not generated in C++, only in C.

[clang] support Wold-style-declaration as gcc

773b14c

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 20, 2024

hstk30-hw requested review from sam-mccall, aaronpuchert, cor3ntin and AaronBallman and removed request for sam-mccall January 20, 2024 09:27

SihangZhu changed the title ~~[clang] support Wold-style-declaration as gcc~~ [clang] support Wold-style-declaration Jan 20, 2024

Merge branch 'main' into old-style-declaration

c1f6e0c

thesamesam added the clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer label Jan 24, 2024

AaronBallman reviewed Feb 20, 2024

View reviewed changes

AaronBallman added the c label Feb 20, 2024

AaronBallman reviewed Feb 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] support Wold-style-declaration #78837

[clang] support Wold-style-declaration #78837

SihangZhu commented Jan 20, 2024 •

edited

llvmbot commented Jan 20, 2024

SihangZhu commented Jan 24, 2024

SihangZhu commented Feb 19, 2024

AaronBallman left a comment

AaronBallman Feb 20, 2024

AaronBallman Feb 20, 2024

AaronBallman Feb 20, 2024

AaronBallman Feb 20, 2024

AaronBallman Feb 20, 2024

AaronBallman Feb 20, 2024

AaronBallman Feb 20, 2024

		def warn_old_style_declaration: Warning <"'%0' is not at beginning of declaration">,
		InGroup<OldStyleDeclaration>, DefaultIgnore;

		auto int x6 = 0;
		int auto x7 = 0; // expected-warning {{'auto' is not at beginning of declaration}}

[clang] support Wold-style-declaration #78837

Are you sure you want to change the base?

[clang] support Wold-style-declaration #78837

Conversation

SihangZhu commented Jan 20, 2024 • edited

llvmbot commented Jan 20, 2024

SihangZhu commented Jan 24, 2024

SihangZhu commented Feb 19, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

AaronBallman Feb 20, 2024

Choose a reason for hiding this comment

AaronBallman Feb 20, 2024

Choose a reason for hiding this comment

AaronBallman Feb 20, 2024

Choose a reason for hiding this comment

AaronBallman Feb 20, 2024

Choose a reason for hiding this comment

AaronBallman Feb 20, 2024

Choose a reason for hiding this comment

AaronBallman Feb 20, 2024

Choose a reason for hiding this comment

AaronBallman Feb 20, 2024

Choose a reason for hiding this comment

SihangZhu commented Jan 20, 2024 •

edited