On 16/06/2014 19:49, David Majnemer wrote:
On Sun, Jun 15, 2014 at 8:11 PM, Alp Toker <[email protected]
<mailto:[email protected]>> 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
[email protected] <mailto:[email protected]>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
--
http://www.nuanti.com
the browser experts
--
http://www.nuanti.com
the browser experts
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
