[PATCH] Sema: Static redeclaration after extern declarations is a Microsoft Extension
Alp Toker
alp at nuanti.com
Mon Jun 16 10:00:11 PDT 2014
On 16/06/2014 19:49, David Majnemer wrote:
>
>
>
> On Sun, Jun 15, 2014 at 8:11 PM, Alp Toker <alp at nuanti.com
> <mailto: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?
err_deleted_decl_not_first possibly.
>
> + 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.
Okay, and I prefer named inputs and outputs where possible and ease of
adding additional outputs if needed :-)
Will leave it to your judgement.
>
> +}
> +
> /// 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.
NoteID sounds much better. PrevDiag refers to "the previous diagnostic"
in at least one place and that usage makes more sense to me. This one
holds the ID of the note diagnostic we want.
Alp.
>
> // 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 <mailto:cfe-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
> --
> http://www.nuanti.com
> the browser experts
>
>
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list