On Mon, Jun 2, 2014 at 8:40 PM, Alp Toker <[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.
> 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.
What about the check do you think is wrong? 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]
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
> --
> http://www.nuanti.com
> the browser experts
>
>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
