<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Jun 15, 2014 at 8:11 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><br>
On 15/06/2014 22:42, David Majnemer wrote:<br>
</div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="h5">
Hi rsmith,<br>
<br>
CL permits static redeclarations to follow extern declarations. The<br>
storage specifier on the latter declaration has no effect.<br>
<br>
This fixes PR20034.<br>
<br>
<a href="http://reviews.llvm.org/D4149" target="_blank">http://reviews.llvm.org/D4149</a><br>
<br>
Files:<br>
include/clang/Basic/<u></u>DiagnosticSemaKinds.td<br>
lib/Sema/SemaDecl.cpp<br>
test/Misc/warning-flags.c<br>
test/Sema/private-extern.c<br>
test/Sema/tentative-decls.c<br>
test/Sema/thread-specifier.c<br>
test/Sema/var-redecl.c<br>
test/SemaCXX/<u></u>MicrosoftExtensions.cpp<br>
<br></div></div>
D4149.10428.patch<br>
<br>
<br>
Index: include/clang/Basic/<u></u>DiagnosticSemaKinds.td<br>
==============================<u></u>==============================<u></u>=======<br>
--- include/clang/Basic/<u></u>DiagnosticSemaKinds.td<br>
+++ include/clang/Basic/<u></u>DiagnosticSemaKinds.td<br>
@@ -3809,8 +3809,8 @@<br>
"declared %select{in global scope|with C language linkage}0 here">;<br>
def warn_weak_import : Warning <<br>
"an already-declared variable is made a weak_import declaration %0">;<br>
-def warn_static_non_static : ExtWarn<<br>
- "static declaration of %0 follows non-static declaration">;<br>
+def ext_static_non_static : Extension<<br>
+ "redeclaring non-static %0 as static is a Microsoft extension">, InGroup<Microsoft>;<br>
def err_non_static_static : Error<<br>
"non-static declaration of %0 follows static declaration">;<br>
def err_extern_non_extern : Error<<br>
Index: lib/Sema/SemaDecl.cpp<br>
==============================<u></u>==============================<u></u>=======<br>
--- lib/Sema/SemaDecl.cpp<br>
+++ lib/Sema/SemaDecl.cpp<br>
@@ -2254,6 +2254,24 @@<br>
return Sema::CXXInvalid;<br>
}<br>
+// Determine whether the previous declaration was a definition, implicit<br>
+// declaration, or a declaration.<br>
+template <typename T><br>
+static std::pair<diag::kind, SourceLocation><br>
+<u></u>getNoteDiagForInvalidRedeclara<u></u>tion(const T *Old, const T *New) {<br>
</blockquote>
<br>
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?<br></blockquote><div><br></div><div>Where in SemaDeclCXX are you referring to?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ diag::kind PrevDiag;<br>
+ SourceLocation OldLocation = Old->getLocation();<br>
+ if (Old-><u></u>isThisDeclarationADefinition()<u></u>)<br>
+ PrevDiag = diag::note_previous_<u></u>definition;<br>
+ else if (Old->isImplicit()) {<br>
+ PrevDiag = diag::note_previous_implicit_<u></u>declaration;<br>
+ if (OldLocation.isInvalid())<br>
+ OldLocation = New->getLocation();<br>
+ } else<br>
+ PrevDiag = diag::note_previous_<u></u>declaration;<br>
+ return std::make_pair(PrevDiag, OldLocation);<br>
</blockquote>
<br>
The use of a std::pair here isn't convincing to me. Would a couple of reference parameters (&NoteID, &PrevLocation) be more descriptive?<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+}<br>
+<br>
/// canRedefineFunction - checks if a function can be redefined. Currently,<br>
/// only extern inline functions can be redefined, and even then only in<br>
/// GNU89 mode.<br>
@@ -2346,18 +2364,10 @@<br>
if (Old->isInvalidDecl())<br>
return true;<br>
- // Determine whether the previous declaration was a definition,<br>
- // implicit declaration, or a declaration.<br>
diag::kind PrevDiag;<br>
- SourceLocation OldLocation = Old->getLocation();<br>
- if (Old-><u></u>isThisDeclarationADefinition()<u></u>)<br>
- PrevDiag = diag::note_previous_<u></u>definition;<br>
- else if (Old->isImplicit()) {<br>
- PrevDiag = diag::note_previous_implicit_<u></u>declaration;<br>
- if (OldLocation.isInvalid())<br>
- OldLocation = New->getLocation();<br>
- } else<br>
- PrevDiag = diag::note_previous_<u></u>declaration;<br>
+ SourceLocation OldLocation;<br>
+ std::tie(PrevDiag, OldLocation) =<br>
+ getNoteDiagForInvalidRedeclara<u></u>tion(Old, New);<br>
</blockquote>
<br>
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?<br></blockquote><div>
<br></div><div>AFAICT, PrevDiag is only used for referring to a previous declaration. I see it used elsewhere for diag::note_previous_builtin_declaration.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
// Don't complain about this if we're in GNU89 mode and the old function<br>
// is an extern inline function.<br>
@@ -2369,7 +2379,7 @@<br>
!New-><u></u>getTemplateSpecializationInfo(<u></u>) &&<br>
!canRedefineFunction(Old, getLangOpts())) {<br>
if (getLangOpts().MicrosoftExt) {<br>
- Diag(New->getLocation(), diag::warn_static_non_static) << New;<br>
+ Diag(New->getLocation(), diag::ext_static_non_static) << New;<br>
Diag(OldLocation, PrevDiag);<br>
} else {<br>
Diag(New->getLocation(), diag::err_static_non_static) << New;<br>
@@ -3070,13 +3080,25 @@<br>
if (New->isInvalidDecl())<br>
return;<br>
+ diag::kind PrevDiag;<br>
+ SourceLocation OldLocation;<br>
+ std::tie(PrevDiag, OldLocation) =<br>
+ getNoteDiagForInvalidRedeclara<u></u>tion(Old, New);<br>
+<br>
// [dcl.stc]p8: Check if we have a non-static decl followed by a static.<br>
if (New->getStorageClass() == SC_Static &&<br>
!New->isStaticDataMember() &&<br>
Old->hasExternalFormalLinkage(<u></u>)) {<br>
- Diag(New->getLocation(), diag::err_static_non_static) << New->getDeclName();<br>
- Diag(Old->getLocation(), diag::note_previous_<u></u>definition);<br>
- return New->setInvalidDecl();<br>
+ if (getLangOpts().MicrosoftExt) {<br>
+ Diag(New->getLocation(), diag::ext_static_non_static)<br>
+ << New->getDeclName();<br>
+ Diag(OldLocation, PrevDiag);<br>
+ } else {<br>
+ Diag(New->getLocation(), diag::err_static_non_static)<br>
+ << New->getDeclName();<br>
+ Diag(OldLocation, PrevDiag);<br>
+ return New->setInvalidDecl();<br>
+ }<br>
}<br>
</blockquote>
<br>
This warn/ext/err pattern is unfortunate. Guess there's not much we can do about it though.<br>
<br>
Alp.<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
// C99 6.2.2p4:<br>
// For an identifier declared with the storage-class specifier<br>
@@ -3093,21 +3115,21 @@<br>
!New->isStaticDataMember() &&<br>
Old->getCanonicalDecl()-><u></u>getStorageClass() == SC_Static) {<br>
Diag(New->getLocation(), diag::err_non_static_static) << New->getDeclName();<br>
- Diag(Old->getLocation(), diag::note_previous_<u></u>definition);<br>
+ Diag(OldLocation, PrevDiag);<br>
return New->setInvalidDecl();<br>
}<br>
// Check if extern is followed by non-extern and vice-versa.<br>
if (New->hasExternalStorage() &&<br>
!Old->hasLinkage() && Old->isLocalVarDecl()) {<br>
Diag(New->getLocation(), diag::err_extern_non_extern) << New->getDeclName();<br>
- Diag(Old->getLocation(), diag::note_previous_<u></u>definition);<br>
+ Diag(OldLocation, PrevDiag);<br>
return New->setInvalidDecl();<br>
}<br>
if (Old->hasLinkage() && New->isLocalVarDecl() &&<br>
!New->hasExternalStorage()) {<br>
Diag(New->getLocation(), diag::err_non_extern_extern) << New->getDeclName();<br>
- Diag(Old->getLocation(), diag::note_previous_<u></u>definition);<br>
+ Diag(OldLocation, PrevDiag);<br>
return New->setInvalidDecl();<br>
}<br>
@@ -3120,25 +3142,25 @@<br>
!(Old->getLexicalDeclContext()<u></u>->isRecord() &&<br>
!New->getLexicalDeclContext()-<u></u>>isRecord())) {<br>
Diag(New->getLocation(), diag::err_redefinition) << New->getDeclName();<br>
- Diag(Old->getLocation(), diag::note_previous_<u></u>definition);<br>
+ Diag(OldLocation, PrevDiag);<br>
return New->setInvalidDecl();<br>
}<br>
if (New->getTLSKind() != Old->getTLSKind()) {<br>
if (!Old->getTLSKind()) {<br>
Diag(New->getLocation(), diag::err_thread_non_thread) << New->getDeclName();<br>
- Diag(Old->getLocation(), diag::note_previous_<u></u>declaration);<br>
+ Diag(OldLocation, PrevDiag);<br>
} else if (!New->getTLSKind()) {<br>
Diag(New->getLocation(), diag::err_non_thread_thread) << New->getDeclName();<br>
- Diag(Old->getLocation(), diag::note_previous_<u></u>declaration);<br>
+ Diag(OldLocation, PrevDiag);<br>
} else {<br>
// Do not allow redeclaration to change the variable between requiring<br>
// static and dynamic initialization.<br>
// FIXME: GCC allows this, but uses the TLS keyword on the first<br>
// declaration to determine the kind. Do we need to be compatible here?<br>
Diag(New->getLocation(), diag::err_thread_thread_<u></u>different_kind)<br>
<< New->getDeclName() << (New->getTLSKind() == VarDecl::TLS_Dynamic);<br>
- Diag(Old->getLocation(), diag::note_previous_<u></u>declaration);<br>
+ Diag(OldLocation, PrevDiag);<br>
}<br>
}<br>
@@ -3155,7 +3177,7 @@<br>
if (<u></u>haveIncompatibleLanguageLinkag<u></u>es(Old, New)) {<br>
Diag(New->getLocation(), diag::err_different_language_<u></u>linkage) << New;<br>
- Diag(Old->getLocation(), diag::note_previous_<u></u>definition);<br>
+ Diag(OldLocation, PrevDiag);<br>
New->setInvalidDecl();<br>
return;<br>
}<br>
Index: test/Misc/warning-flags.c<br>
==============================<u></u>==============================<u></u>=======<br>
--- test/Misc/warning-flags.c<br>
+++ test/Misc/warning-flags.c<br>
@@ -18,7 +18,7 @@<br>
The list of warnings below should NEVER grow. It should gradually shrink to 0.<br>
-CHECK: Warnings without flags (106):<br>
+CHECK: Warnings without flags (105):<br>
CHECK-NEXT: ext_delete_void_ptr_operand<br>
CHECK-NEXT: ext_expected_semi_decl_list<br>
CHECK-NEXT: ext_explicit_specialization_<u></u>storage_class<br>
@@ -114,7 +114,6 @@<br>
CHECK-NEXT: warn_register_objc_catch_parm<br>
CHECK-NEXT: warn_related_result_type_<u></u>compatibility_class<br>
CHECK-NEXT: warn_related_result_type_<u></u>compatibility_protocol<br>
-CHECK-NEXT: warn_static_non_static<br>
CHECK-NEXT: warn_template_export_<u></u>unsupported<br>
CHECK-NEXT: warn_template_spec_extra_<u></u>headers<br>
CHECK-NEXT: warn_tentative_incomplete_<u></u>array<br>
Index: test/Sema/private-extern.c<br>
==============================<u></u>==============================<u></u>=======<br>
--- test/Sema/private-extern.c<br>
+++ test/Sema/private-extern.c<br>
@@ -13,10 +13,10 @@<br>
int g3; // expected-note{{previous definition}}<br>
static int g3; // expected-error{{static declaration of 'g3' follows non-static declaration}}<br>
-extern int g4; // expected-note{{previous definition}}<br>
+extern int g4; // expected-note{{previous declaration}}<br>
static int g4; // expected-error{{static declaration of 'g4' follows non-static declaration}}<br>
-__private_extern__ int g5; // expected-note{{previous definition}}<br>
+__private_extern__ int g5; // expected-note{{previous declaration}}<br>
static int g5; // expected-error{{static declaration of 'g5' follows non-static declaration}}<br>
void f0() {<br>
@@ -30,12 +30,12 @@<br>
}<br>
void f2() {<br>
- extern int g8; // expected-note{{previous definition}}<br>
+ extern int g8; // expected-note{{previous declaration}}<br>
int g8; // expected-error {{non-extern declaration of 'g8' follows extern declaration}}<br>
}<br>
void f3() {<br>
- __private_extern__ int g9; // expected-note{{previous definition}}<br>
+ __private_extern__ int g9; // expected-note{{previous declaration}}<br>
int g9; // expected-error {{non-extern declaration of 'g9' follows extern declaration}}<br>
}<br>
Index: test/Sema/tentative-decls.c<br>
==============================<u></u>==============================<u></u>=======<br>
--- test/Sema/tentative-decls.c<br>
+++ test/Sema/tentative-decls.c<br>
@@ -23,7 +23,7 @@<br>
int i1 = 2; // expected-error {{redefinition of 'i1'}}<br>
int i1;<br>
int i1;<br>
-extern int i5; // expected-note {{previous definition is here}}<br>
+extern int i5; // expected-note {{previous declaration is here}}<br>
static int i5; // expected-error{{static declaration of 'i5' follows non-static declaration}}<br>
static int i2 = 5; // expected-note 1 {{previous definition is here}}<br>
@@ -49,7 +49,7 @@<br>
int redef[11]; // expected-error{{redefinition of 'redef'}}<br>
void func() {<br>
- extern int i6; // expected-note {{previous definition is here}}<br>
+ extern int i6; // expected-note {{previous declaration is here}}<br>
static int i6; // expected-error{{static declaration of 'i6' follows non-static declaration}}<br>
}<br>
Index: test/Sema/thread-specifier.c<br>
==============================<u></u>==============================<u></u>=======<br>
--- test/Sema/thread-specifier.c<br>
+++ test/Sema/thread-specifier.c<br>
@@ -58,7 +58,7 @@<br>
}<br>
__thread typedef int t14; // expected-error-re {{cannot combine with previous '{{__thread|_Thread_local|<u></u>thread_local}}' declaration specifier}}<br>
-__thread int t15; // expected-note {{previous declaration is here}}<br>
+__thread int t15; // expected-note {{previous definition is here}}<br>
extern int t15; // expected-error {{non-thread-local declaration of 't15' follows thread-local declaration}}<br>
extern int t16; // expected-note {{previous declaration is here}}<br>
__thread int t16; // expected-error {{thread-local declaration of 't16' follows non-thread-local declaration}}<br>
Index: test/Sema/var-redecl.c<br>
==============================<u></u>==============================<u></u>=======<br>
--- test/Sema/var-redecl.c<br>
+++ test/Sema/var-redecl.c<br>
@@ -58,5 +58,5 @@<br>
// PR3645<br>
static int a;<br>
-extern int a; // expected-note {{previous definition is here}}<br>
+extern int a; // expected-note {{previous declaration is here}}<br>
int a; // expected-error {{non-static declaration of 'a' follows static declaration}}<br>
Index: test/SemaCXX/<u></u>MicrosoftExtensions.cpp<br>
==============================<u></u>==============================<u></u>=======<br>
--- test/SemaCXX/<u></u>MicrosoftExtensions.cpp<br>
+++ test/SemaCXX/<u></u>MicrosoftExtensions.cpp<br>
@@ -144,11 +144,14 @@<br>
void static_func(); // expected-note {{previous declaration is here}}<br>
-static void static_func() // expected-warning {{static declaration of 'static_func' follows non-static declaration}}<br>
+static void static_func() // expected-warning {{redeclaring non-static 'static_func' as static is a Microsoft extension}}<br>
{<br>
}<br>
+extern const int static_var; // expected-note {{previous declaration is here}}<br>
+static const int static_var = 3; // expected-warning {{redeclaring non-static 'static_var' as static is a Microsoft extension}}<br>
+<br>
long function_prototype(int a);<br>
long (*function_ptr)(int a);<br>
<br>
<br>
______________________________<u></u>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><span class=""><font color="#888888"><br>
</font></span></blockquote><span class=""><font color="#888888">
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</font></span></blockquote></div><br></div></div>