[PATCH] Downgrade "definition of dllimport static field" error to warning for class templates (PR19902)

[Hans Wennborg]

Thanks for the review!

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,
>
>
> ?

Why the question mark? Nico and Reid are familiar with this code, I
figured they'd be good reviewers and so I CC'd them in Phabricator.

>> 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...

It's tricky. I'd like to say it's ill-formed, but the problem is that
MSVC accepts it unless the template is instantiated.

Ideally, we should just reject that code, as Clang does today, but
it's used in system headers so we'll have to work around it somehow.

My patch doesn't actually make us ignore the attribute. We'll end up
codegening this as "available_externally dllimport". Ignoring the
attribute would not be good since we don't want to emit a definition.
I'll add a codegen test.

>>   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).

Hmm, yes maybe a DefaultError warning would be better. I agree my
patch is a little clumsy. I'd like to hear what Reid or Nick think
about that.

>> +++ 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.

Sure, if we just always make it a warning it gets much simpler.

Thanks,
Hans