[PATCH] Sema: Static redeclaration after extern declarations is a Microsoft Extension
David Majnemer
david.majnemer at gmail.com
Mon Jun 16 09:49:44 PDT 2014
On Sun, Jun 15, 2014 at 8:11 PM, Alp Toker <alp at nuanti.com> wrote:
>
> On 15/06/2014 22:42, David Majnemer wrote:
>
>> Hi rsmith,
>>
>> CL permits static redeclarations to follow extern declarations. The
>> storage specifier on the latter declaration has no effect.
>>
>> This fixes PR20034.
>>
>> http://reviews.llvm.org/D4149
>>
>> Files:
>> include/clang/Basic/DiagnosticSemaKinds.td
>> lib/Sema/SemaDecl.cpp
>> test/Misc/warning-flags.c
>> test/Sema/private-extern.c
>> test/Sema/tentative-decls.c
>> test/Sema/thread-specifier.c
>> test/Sema/var-redecl.c
>> test/SemaCXX/MicrosoftExtensions.cpp
>>
>> D4149.10428.patch
>>
>>
>> Index: include/clang/Basic/DiagnosticSemaKinds.td
>> ===================================================================
>> --- include/clang/Basic/DiagnosticSemaKinds.td
>> +++ include/clang/Basic/DiagnosticSemaKinds.td
>> @@ -3809,8 +3809,8 @@
>> "declared %select{in global scope|with C language linkage}0 here">;
>> def warn_weak_import : Warning <
>> "an already-declared variable is made a weak_import declaration %0">;
>> -def warn_static_non_static : ExtWarn<
>> - "static declaration of %0 follows non-static declaration">;
>> +def ext_static_non_static : Extension<
>> + "redeclaring non-static %0 as static is a Microsoft extension">,
>> InGroup<Microsoft>;
>> def err_non_static_static : Error<
>> "non-static declaration of %0 follows static declaration">;
>> def err_extern_non_extern : Error<
>> Index: lib/Sema/SemaDecl.cpp
>> ===================================================================
>> --- lib/Sema/SemaDecl.cpp
>> +++ lib/Sema/SemaDecl.cpp
>> @@ -2254,6 +2254,24 @@
>> return Sema::CXXInvalid;
>> }
>> +// Determine whether the previous declaration was a definition,
>> implicit
>> +// declaration, or a declaration.
>> +template <typename T>
>> +static std::pair<diag::kind, SourceLocation>
>> +getNoteDiagForInvalidRedeclaration(const T *Old, const T *New) {
>>
>
> Nice utility function. There's at least one place in SemaDeclCXX.cpp that
> could use this too. Wonder if it's worth putting in a header?
>
Where in SemaDeclCXX are you referring to?
>
> + diag::kind PrevDiag;
>> + SourceLocation OldLocation = Old->getLocation();
>> + if (Old->isThisDeclarationADefinition())
>> + PrevDiag = diag::note_previous_definition;
>> + else if (Old->isImplicit()) {
>> + PrevDiag = diag::note_previous_implicit_declaration;
>> + if (OldLocation.isInvalid())
>> + OldLocation = New->getLocation();
>> + } else
>> + PrevDiag = diag::note_previous_declaration;
>> + return std::make_pair(PrevDiag, OldLocation);
>>
>
> The use of a std::pair here isn't convincing to me. Would a couple of
> reference parameters (&NoteID, &PrevLocation) be more descriptive?
>
I don't like having multiple "out" parameters if I can easily avoid it.
Returning std::pair here represents my intent well and is not without
precedent.
>
> +}
>> +
>> /// canRedefineFunction - checks if a function can be redefined.
>> Currently,
>> /// only extern inline functions can be redefined, and even then only in
>> /// GNU89 mode.
>> @@ -2346,18 +2364,10 @@
>> if (Old->isInvalidDecl())
>> return true;
>> - // Determine whether the previous declaration was a definition,
>> - // implicit declaration, or a declaration.
>> diag::kind PrevDiag;
>> - SourceLocation OldLocation = Old->getLocation();
>> - if (Old->isThisDeclarationADefinition())
>> - PrevDiag = diag::note_previous_definition;
>> - else if (Old->isImplicit()) {
>> - PrevDiag = diag::note_previous_implicit_declaration;
>> - if (OldLocation.isInvalid())
>> - OldLocation = New->getLocation();
>> - } else
>> - PrevDiag = diag::note_previous_declaration;
>> + SourceLocation OldLocation;
>> + std::tie(PrevDiag, OldLocation) =
>> + getNoteDiagForInvalidRedeclaration(Old, New);
>>
>
> PrevDiag usually refers to a previously emitted diagnostic so it feels
> like the wrong name to use here. I know it was this way before your patch
> but could you rename these to NoteID and PrevLocation?
>
AFAICT, PrevDiag is only used for referring to a previous declaration. I
see it used elsewhere for diag::note_previous_builtin_declaration.
>
> // Don't complain about this if we're in GNU89 mode and the old
>> function
>> // is an extern inline function.
>> @@ -2369,7 +2379,7 @@
>> !New->getTemplateSpecializationInfo() &&
>> !canRedefineFunction(Old, getLangOpts())) {
>> if (getLangOpts().MicrosoftExt) {
>> - Diag(New->getLocation(), diag::warn_static_non_static) << New;
>> + Diag(New->getLocation(), diag::ext_static_non_static) << New;
>> Diag(OldLocation, PrevDiag);
>> } else {
>> Diag(New->getLocation(), diag::err_static_non_static) << New;
>> @@ -3070,13 +3080,25 @@
>> if (New->isInvalidDecl())
>> return;
>> + diag::kind PrevDiag;
>> + SourceLocation OldLocation;
>> + std::tie(PrevDiag, OldLocation) =
>> + getNoteDiagForInvalidRedeclaration(Old, New);
>> +
>> // [dcl.stc]p8: Check if we have a non-static decl followed by a
>> static.
>> if (New->getStorageClass() == SC_Static &&
>> !New->isStaticDataMember() &&
>> Old->hasExternalFormalLinkage()) {
>> - Diag(New->getLocation(), diag::err_static_non_static) <<
>> New->getDeclName();
>> - Diag(Old->getLocation(), diag::note_previous_definition);
>> - return New->setInvalidDecl();
>> + if (getLangOpts().MicrosoftExt) {
>> + Diag(New->getLocation(), diag::ext_static_non_static)
>> + << New->getDeclName();
>> + Diag(OldLocation, PrevDiag);
>> + } else {
>> + Diag(New->getLocation(), diag::err_static_non_static)
>> + << New->getDeclName();
>> + Diag(OldLocation, PrevDiag);
>> + return New->setInvalidDecl();
>> + }
>> }
>>
>
> This warn/ext/err pattern is unfortunate. Guess there's not much we can do
> about it though.
>
> Alp.
>
>
> // C99 6.2.2p4:
>> // For an identifier declared with the storage-class specifier
>> @@ -3093,21 +3115,21 @@
>> !New->isStaticDataMember() &&
>> Old->getCanonicalDecl()->getStorageClass() == SC_Static) {
>> Diag(New->getLocation(), diag::err_non_static_static) <<
>> New->getDeclName();
>> - Diag(Old->getLocation(), diag::note_previous_definition);
>> + Diag(OldLocation, PrevDiag);
>> return New->setInvalidDecl();
>> }
>> // Check if extern is followed by non-extern and vice-versa.
>> if (New->hasExternalStorage() &&
>> !Old->hasLinkage() && Old->isLocalVarDecl()) {
>> Diag(New->getLocation(), diag::err_extern_non_extern) <<
>> New->getDeclName();
>> - Diag(Old->getLocation(), diag::note_previous_definition);
>> + Diag(OldLocation, PrevDiag);
>> return New->setInvalidDecl();
>> }
>> if (Old->hasLinkage() && New->isLocalVarDecl() &&
>> !New->hasExternalStorage()) {
>> Diag(New->getLocation(), diag::err_non_extern_extern) <<
>> New->getDeclName();
>> - Diag(Old->getLocation(), diag::note_previous_definition);
>> + Diag(OldLocation, PrevDiag);
>> return New->setInvalidDecl();
>> }
>> @@ -3120,25 +3142,25 @@
>> !(Old->getLexicalDeclContext()->isRecord() &&
>> !New->getLexicalDeclContext()->isRecord())) {
>> Diag(New->getLocation(), diag::err_redefinition) <<
>> New->getDeclName();
>> - Diag(Old->getLocation(), diag::note_previous_definition);
>> + Diag(OldLocation, PrevDiag);
>> return New->setInvalidDecl();
>> }
>> if (New->getTLSKind() != Old->getTLSKind()) {
>> if (!Old->getTLSKind()) {
>> Diag(New->getLocation(), diag::err_thread_non_thread) <<
>> New->getDeclName();
>> - Diag(Old->getLocation(), diag::note_previous_declaration);
>> + Diag(OldLocation, PrevDiag);
>> } else if (!New->getTLSKind()) {
>> Diag(New->getLocation(), diag::err_non_thread_thread) <<
>> New->getDeclName();
>> - Diag(Old->getLocation(), diag::note_previous_declaration);
>> + Diag(OldLocation, PrevDiag);
>> } else {
>> // Do not allow redeclaration to change the variable between
>> requiring
>> // static and dynamic initialization.
>> // FIXME: GCC allows this, but uses the TLS keyword on the first
>> // declaration to determine the kind. Do we need to be compatible
>> here?
>> Diag(New->getLocation(), diag::err_thread_thread_different_kind)
>> << New->getDeclName() << (New->getTLSKind() ==
>> VarDecl::TLS_Dynamic);
>> - Diag(Old->getLocation(), diag::note_previous_declaration);
>> + Diag(OldLocation, PrevDiag);
>> }
>> }
>> @@ -3155,7 +3177,7 @@
>> if (haveIncompatibleLanguageLinkages(Old, New)) {
>> Diag(New->getLocation(), diag::err_different_language_linkage) <<
>> New;
>> - Diag(Old->getLocation(), diag::note_previous_definition);
>> + Diag(OldLocation, PrevDiag);
>> New->setInvalidDecl();
>> return;
>> }
>> Index: test/Misc/warning-flags.c
>> ===================================================================
>> --- test/Misc/warning-flags.c
>> +++ test/Misc/warning-flags.c
>> @@ -18,7 +18,7 @@
>> The list of warnings below should NEVER grow. It should gradually
>> shrink to 0.
>> -CHECK: Warnings without flags (106):
>> +CHECK: Warnings without flags (105):
>> CHECK-NEXT: ext_delete_void_ptr_operand
>> CHECK-NEXT: ext_expected_semi_decl_list
>> CHECK-NEXT: ext_explicit_specialization_storage_class
>> @@ -114,7 +114,6 @@
>> CHECK-NEXT: warn_register_objc_catch_parm
>> CHECK-NEXT: warn_related_result_type_compatibility_class
>> CHECK-NEXT: warn_related_result_type_compatibility_protocol
>> -CHECK-NEXT: warn_static_non_static
>> CHECK-NEXT: warn_template_export_unsupported
>> CHECK-NEXT: warn_template_spec_extra_headers
>> CHECK-NEXT: warn_tentative_incomplete_array
>> Index: test/Sema/private-extern.c
>> ===================================================================
>> --- test/Sema/private-extern.c
>> +++ test/Sema/private-extern.c
>> @@ -13,10 +13,10 @@
>> int g3; // expected-note{{previous definition}}
>> static int g3; // expected-error{{static declaration of 'g3' follows
>> non-static declaration}}
>> -extern int g4; // expected-note{{previous definition}}
>> +extern int g4; // expected-note{{previous declaration}}
>> static int g4; // expected-error{{static declaration of 'g4' follows
>> non-static declaration}}
>> -__private_extern__ int g5; // expected-note{{previous definition}}
>> +__private_extern__ int g5; // expected-note{{previous declaration}}
>> static int g5; // expected-error{{static declaration of 'g5' follows
>> non-static declaration}}
>> void f0() {
>> @@ -30,12 +30,12 @@
>> }
>> void f2() {
>> - extern int g8; // expected-note{{previous definition}}
>> + extern int g8; // expected-note{{previous declaration}}
>> int g8; // expected-error {{non-extern declaration of 'g8' follows
>> extern declaration}}
>> }
>> void f3() {
>> - __private_extern__ int g9; // expected-note{{previous definition}}
>> + __private_extern__ int g9; // expected-note{{previous declaration}}
>> int g9; // expected-error {{non-extern declaration of 'g9' follows
>> extern declaration}}
>> }
>> Index: test/Sema/tentative-decls.c
>> ===================================================================
>> --- test/Sema/tentative-decls.c
>> +++ test/Sema/tentative-decls.c
>> @@ -23,7 +23,7 @@
>> int i1 = 2; // expected-error {{redefinition of 'i1'}}
>> int i1;
>> int i1;
>> -extern int i5; // expected-note {{previous definition is here}}
>> +extern int i5; // expected-note {{previous declaration is here}}
>> static int i5; // expected-error{{static declaration of 'i5' follows
>> non-static declaration}}
>> static int i2 = 5; // expected-note 1 {{previous definition is here}}
>> @@ -49,7 +49,7 @@
>> int redef[11]; // expected-error{{redefinition of 'redef'}}
>> void func() {
>> - extern int i6; // expected-note {{previous definition is here}}
>> + extern int i6; // expected-note {{previous declaration is here}}
>> static int i6; // expected-error{{static declaration of 'i6' follows
>> non-static declaration}}
>> }
>> Index: test/Sema/thread-specifier.c
>> ===================================================================
>> --- test/Sema/thread-specifier.c
>> +++ test/Sema/thread-specifier.c
>> @@ -58,7 +58,7 @@
>> }
>> __thread typedef int t14; // expected-error-re {{cannot combine with
>> previous '{{__thread|_Thread_local|thread_local}}' declaration
>> specifier}}
>> -__thread int t15; // expected-note {{previous declaration is here}}
>> +__thread int t15; // expected-note {{previous definition is here}}
>> extern int t15; // expected-error {{non-thread-local declaration of
>> 't15' follows thread-local declaration}}
>> extern int t16; // expected-note {{previous declaration is here}}
>> __thread int t16; // expected-error {{thread-local declaration of 't16'
>> follows non-thread-local declaration}}
>> Index: test/Sema/var-redecl.c
>> ===================================================================
>> --- test/Sema/var-redecl.c
>> +++ test/Sema/var-redecl.c
>> @@ -58,5 +58,5 @@
>> // PR3645
>> static int a;
>> -extern int a; // expected-note {{previous definition is here}}
>> +extern int a; // expected-note {{previous declaration is here}}
>> int a; // expected-error {{non-static declaration of 'a' follows
>> static declaration}}
>> Index: test/SemaCXX/MicrosoftExtensions.cpp
>> ===================================================================
>> --- test/SemaCXX/MicrosoftExtensions.cpp
>> +++ test/SemaCXX/MicrosoftExtensions.cpp
>> @@ -144,11 +144,14 @@
>> void static_func(); // expected-note {{previous declaration is here}}
>> -static void static_func() // expected-warning {{static declaration
>> of 'static_func' follows non-static declaration}}
>> +static void static_func() // expected-warning {{redeclaring non-static
>> 'static_func' as static is a Microsoft extension}}
>> {
>> }
>> +extern const int static_var; // expected-note {{previous declaration
>> is here}}
>> +static const int static_var = 3; // expected-warning {{redeclaring
>> non-static 'static_var' as static is a Microsoft extension}}
>> +
>> long function_prototype(int a);
>> long (*function_ptr)(int a);
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140616/935aa4fe/attachment.html>
More information about the cfe-commits
mailing list