[PATCH] Downgrade "definition of dllimport static field" error to warning for class templates (PR19902)
Reid Kleckner
rnk at google.com
Tue Jun 3 15:10:22 PDT 2014
On Mon, Jun 2, 2014 at 8:40 PM, Alp Toker <alp at nuanti.com> 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 at -1{{definition of dllimport static field
>> not allowed}}
>> +
>> // expected-warning at -1{{definition of dllimport static
>> field not allowed}}
>>
>> // expected-note at -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 at -1{{definition of dllimport static field
>> not allowed}}
>> +
>> // expected-warning at -1{{definition of dllimport static
>> field not allowed}}
>>
>> // expected-note at -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 at -1{{definition of dllimport static field
>> not allowed}}
>> +
>> // expected-warning at -1{{definition of dllimport static
>> field not allowed}}
>>
>> // expected-note at -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 at -1{{definition of
>> dllimport static field not allowed}}
>> +
>> // expected-warning at -1{{definition
>> of dllimport static field not allowed}}
>>
>> // expected-note at -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 at -1{{definition of
>> dllimport static field not allowed}}
>> +
>> // expected-warning at -1{{definition
>> of dllimport static field not allowed}}
>>
>> // expected-note at -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 at -1{{definition of
>> dllimport static field not allowed}}
>> +
>> // expected-warning at -1{{definition
>> of dllimport static field not allowed}}
>>
>> // expected-note at -2{{attribute is
>> here}}
>> #endif // __has_feature(cxx_variable_templates)
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140603/0ff035b7/attachment.html>
More information about the cfe-commits
mailing list