On 04/06/2014 01:10, Reid Kleckner wrote:
On Mon, Jun 2, 2014 at 8:40 PM, Alp Toker <[email protected]
<mailto:[email protected]>> wrote:
On 03/06/2014 05:37, Hans Wennborg wrote:
Hi nrieck, rnk,
?
This allows us to compile the following kind of code, which
occurs in MSVC headers:
template <typename> struct S {
__declspec(dllimport) static int x;
};
template <typename T> int S<T>::x;
Based on your description it sounds like this isn't ill-formed but
the attribute is just ignored. Review inline...
Please take a look.
http://reviews.llvm.org/D3998
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaDecl.cpp
test/SemaCXX/dllimport.cpp
D3998.10038.patch
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2109,6 +2109,9 @@
"definition of dllimport data">;
def err_attribute_dllimport_static_field_definition : Error<
"definition of dllimport static field not allowed">;
+def warn_attribute_dllimport_static_field_definition : Warning<
+ "definition of dllimport static field not allowed">,
+ InGroup<DiagGroup<"dllimport-static-field-def">>;
If that's the case, just make this a warning, or at most, a
DefaultError warning (that will be ignored in system headers).
The non-template case should definitely remain an error.
What does MSVC do? Reject, accept or warn?
def err_attribute_dll_member_of_dll_class : Error<
"attribute %q0 cannot be applied to member of %q1 class">;
def err_attribute_weakref_not_static : Error<
Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -9061,10 +9061,17 @@
if (const DLLImportAttr *IA = VD->getAttr<DLLImportAttr>()) {
if (VD->isStaticDataMember() && VD->isOutOfLine() &&
VD->isThisDeclarationADefinition()) {
+ // Definitions of dllimport class template static data
members occurs
+ // in MSVC header files. Downgrade to a warning.
+ bool JustWarn =
cast<CXXRecordDecl>(VD->getFirstDecl()->getDeclContext())
+ ->getDescribedClassTemplate();
+
There is no need for that check and it doesn't look right.
The way I think about these definitions of static data members of
not-fully-specialized class templates is that they're like inline
functions. I don't see any reason why you can't provide a definition
out of line while still importing the definition from another DLL. It
should get available_externally linkage. If it's not const, it's not
very useful for optimization purposes, but it seems semantically
consistent.
If it's perfectly OK and semantically consistent to accept this then why
emit a warning?
What about the check do you think is wrong?
We'd normally check linkage/inlining or use some other existing
predicate for a check like this instead of poking at the decl for a
specific special case.
Also note the patch was described as a system header workaround, while
you're talking about accepting it as fully valid user code. The correct
way to implement this patch depends on which of the two is preferable --
so that needs to be decided on first.
Alp.
The one case I'd want to make sure we still error on is the
out-of-line definition of a static data member of a fully-specialized
class template:
template <typename T> struct S { static int x; };
template <typename T> int S<T>::x = sizeof(T);
template <> struct __declspec(dllimport) S<int> { static int x; };
int S<int>::x = -1; // should error, only the dll should have this
definition.
Diag(VD->getLocation(),
-
diag::err_attribute_dllimport_static_field_definition);
+ JustWarn ?
diag::warn_attribute_dllimport_static_field_definition
+ :
diag::err_attribute_dllimport_static_field_definition);
Diag(IA->getLocation(), diag::note_attribute);
- VD->setInvalidDecl();
+ if (!JustWarn)
+ VD->setInvalidDecl();
Assuming the code isn't ill-formed, the declaration should not be
set invalid at all.
Alp.
}
}
Index: test/SemaCXX/dllimport.cpp
===================================================================
--- test/SemaCXX/dllimport.cpp
+++ test/SemaCXX/dllimport.cpp
@@ -816,9 +816,9 @@
template<typename T> inline void
ImportClassTmplMembers<T>::staticInlineDef() {}
template<typename T> void
ImportClassTmplMembers<T>::staticInlineDecl() {}
-template<typename T> int
ImportClassTmplMembers<T>::StaticFieldDef; //
expected-error{{definition of dllimport static field not allowed}}
-template<typename T> const int
ImportClassTmplMembers<T>::StaticConstFieldDef = 1; //
expected-error{{definition of dllimport static field not allowed}}
-template<typename T> constexpr int
ImportClassTmplMembers<T>::ConstexprFieldDef; //
expected-error{{definition of dllimport static field not allowed}}
+template<typename T> int
ImportClassTmplMembers<T>::StaticFieldDef; //
expected-warning{{definition of dllimport static field not
allowed}}
+template<typename T> const int
ImportClassTmplMembers<T>::StaticConstFieldDef = 1; //
expected-warning{{definition of dllimport static field not
allowed}}
+template<typename T> constexpr int
ImportClassTmplMembers<T>::ConstexprFieldDef; //
expected-warning{{definition of dllimport static field not
allowed}}
// Redeclarations cannot add dllimport.
@@ -853,13 +853,13 @@
template<typename T> __declspec(dllimport) void
CTMR<T>::staticInlineDecl() {} //
expected-error{{redeclaration of 'CTMR::staticInlineDecl'
cannot add 'dllimport' attribute}}
template<typename T> __declspec(dllimport) int
CTMR<T>::StaticField = 1; //
expected-error{{redeclaration of 'CTMR::StaticField' cannot
add 'dllimport' attribute}}
- //
expected-error@-1{{definition of dllimport static field not
allowed}}
+ //
expected-warning@-1{{definition of dllimport static field not
allowed}}
//
expected-note@-2{{attribute is here}}
template<typename T> __declspec(dllimport) const int
CTMR<T>::StaticConstField = 1; //
expected-error{{redeclaration of 'CTMR::StaticConstField'
cannot add 'dllimport' attribute}}
- //
expected-error@-1{{definition of dllimport static field not
allowed}}
+ //
expected-warning@-1{{definition of dllimport static field not
allowed}}
//
expected-note@-2{{attribute is here}}
template<typename T> __declspec(dllimport) constexpr int
CTMR<T>::ConstexprField; // expected-error{{redeclaration
of 'CTMR::ConstexprField' cannot add 'dllimport' attribute}}
- //
expected-error@-1{{definition of dllimport static field not
allowed}}
+ //
expected-warning@-1{{definition of dllimport static field not
allowed}}
//
expected-note@-2{{attribute is here}}
@@ -901,9 +901,9 @@
template<typename T> template<typename U> void
ImportClsTmplMemTmpl<T>::staticInlineDecl() {}
#if __has_feature(cxx_variable_templates)
-template<typename T> template<typename U> int
ImportClsTmplMemTmpl<T>::StaticFieldDef; //
expected-error{{definition of dllimport static field not allowed}}
-template<typename T> template<typename U> const int
ImportClsTmplMemTmpl<T>::StaticConstFieldDef = 1; //
expected-error{{definition of dllimport static field not allowed}}
-template<typename T> template<typename U> constexpr int
ImportClsTmplMemTmpl<T>::ConstexprFieldDef; //
expected-error{{definition of dllimport static field not allowed}}
+template<typename T> template<typename U> int
ImportClsTmplMemTmpl<T>::StaticFieldDef; //
expected-warning{{definition of dllimport static field not
allowed}}
+template<typename T> template<typename U> const int
ImportClsTmplMemTmpl<T>::StaticConstFieldDef = 1; //
expected-warning{{definition of dllimport static field not
allowed}}
+template<typename T> template<typename U> constexpr int
ImportClsTmplMemTmpl<T>::ConstexprFieldDef; //
expected-warning{{definition of dllimport static field not
allowed}}
#endif // __has_feature(cxx_variable_templates)
@@ -935,13 +935,13 @@
#if __has_feature(cxx_variable_templates)
template<typename T> template<typename U>
__declspec(dllimport) int CTMTR<T>::StaticField = 1;
// expected-error{{redeclaration of 'CTMTR::StaticField'
cannot add 'dllimport' attribute}}
- //
expected-error@-1{{definition of dllimport static field not
allowed}}
+ //
expected-warning@-1{{definition of dllimport static field not
allowed}}
// expected-note@-2{{attribute is here}}
template<typename T> template<typename U>
__declspec(dllimport) const int CTMTR<T>::StaticConstField =
1; // expected-error{{redeclaration of
'CTMTR::StaticConstField' cannot add 'dllimport' attribute}}
- //
expected-error@-1{{definition of dllimport static field not
allowed}}
+ //
expected-warning@-1{{definition of dllimport static field not
allowed}}
// expected-note@-2{{attribute is here}}
template<typename T> template<typename U>
__declspec(dllimport) constexpr int CTMTR<T>::ConstexprField;
// expected-error{{redeclaration of
'CTMTR::ConstexprField' cannot add 'dllimport' attribute}}
- //
expected-error@-1{{definition of dllimport static field not
allowed}}
+ //
expected-warning@-1{{definition of dllimport static field not
allowed}}
// expected-note@-2{{attribute is here}}
#endif // __has_feature(cxx_variable_templates)
_______________________________________________
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
