[PATCH] Downgrade "definition of dllimport static field" error to warning for class templates (PR19902)
Alp Toker
alp at nuanti.com
Tue Jun 3 15:36:52 PDT 2014
On 04/06/2014 01:10, Reid Kleckner wrote:
>
>
>
> On Mon, Jun 2, 2014 at 8:40 PM, Alp Toker <alp at nuanti.com
> <mailto: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.
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 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 <mailto:cfe-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
> --
> http://www.nuanti.com
> the browser experts
>
>
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list