Skip to content

[clang] support Wold-style-declaration #78837

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[clang] support Wold-style-declaration #78837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
773b14c
[clang] support Wold-style-declaration as gcc
SihangZhu Jan 20, 2024
c1f6e0c
Merge branch 'main' into old-style-declaration
SihangZhu Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/include/clang/Basic/DiagnosticCommonKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,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;
Comment on lines +91 to +92
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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">;
Expand Down
2 changes: 2 additions & 0 deletions clang/include/clang/Basic/DiagnosticGroups.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">;
Expand Down Expand Up @@ -1026,6 +1027,7 @@ def Extra : DiagGroup<"extra", [
EmptyInitStatement,
StringConcatation,
FUseLdPath,
OldStyleDeclaration,
]>;

def Most : DiagGroup<"most", [
Expand Down
11 changes: 11 additions & 0 deletions clang/lib/Parse/ParseDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3348,6 +3348,7 @@ void Parser::ParseDeclarationSpecifiers(
while (true) {
bool isInvalid = false;
bool isStorageClass = false;
bool isFunctionSpecifier = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const char *PrevSpec = nullptr;
unsigned DiagID = 0;

Expand Down Expand Up @@ -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
Expand All @@ -4104,6 +4106,7 @@ void Parser::ParseDeclarationSpecifiers(
isInvalid = true;
} else {
isInvalid = DS.setFunctionSpecVirtual(Loc, PrevSpec, DiagID);
isFunctionSpecifier = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
break;
case tok::kw_explicit: {
Expand Down Expand Up @@ -4140,12 +4143,14 @@ void Parser::ParseDeclarationSpecifiers(
}
isInvalid = DS.setFunctionSpecExplicit(ExplicitLoc, PrevSpec, DiagID,
ExplicitSpec, CloseParenLoc);
isFunctionSpecifier = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C also doesn't have explicit.

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
Expand Down Expand Up @@ -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.
Expand Down
33 changes: 33 additions & 0 deletions clang/test/Parser/old-style-declaration.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// RUN: %clang_cc1 -fsyntax-only -verify -Wold-style-declaration %s
// RUN: %clang_cc1 -fsyntax-only -verify -Wextra %s

Comment on lines +2 to +3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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}}
Comment on lines +18 to +19
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.