[patch] Don't drop attributes when checking explicit specializations.

[Nico Weber]

Thanks everyone! I landed this in r224651 since it seems to be
behavior-neutral on visibility (my chromium components build also completed
successfully).

Please let me know if this causes problems for someone.

On Fri, Dec 19, 2014 at 2:25 PM, Richard Smith <richard at metafoo.co.uk>
wrote:
>
> On Fri, Dec 19, 2014 at 12:58 PM, Reid Kleckner <rnk at google.com> wrote:
>>
>> I think it's more interesting when the specialization lacks attributes:
>>
>>     template <int>
>>     struct A { void __attribute__((visibility("hidden"))) foo() {} };
>>     template <> void A<0>::foo() {} // implicitly default visibility
>>
>>     // compile with -fvisibility=hidden
>>     template <int>
>>     struct A { void __attribute__((visibility("default"))) foo() {} };
>>     template <> void A<0>::foo() {} // implicitly hidden visibility
>>
>> I'm fine with this if there's no behavior change here.
>>
>
> In both GCC and Clang, with and without this patch, A<0>::foo takes its
> visibility from the attribute.
>
>
>> On Thu, Dec 18, 2014 at 11:42 PM, Nico Weber <thakis at chromium.org> wrote:
>>
>>> On Thu, Dec 18, 2014 at 9:58 PM, John McCall <rjmccall at apple.com> wrote:
>>>>
>>>> On Dec 18, 2014, at 6:18 PM, Richard Smith <richard at metafoo.co.uk>
>>>> wrote:
>>>> On Wed, Dec 17, 2014 at 6:58 PM, Nico Weber <thakis at chromium.org>
>>>> wrote:
>>>>>
>>>>> Don't drop attributes when checking explicit specializations.
>>>>>
>>>>> Consider a template class with attributes on a method, and an explicit
>>>>> specialization of that method:
>>>>>
>>>>>     template <int>
>>>>>     struct A {
>>>>>       void foo() final;
>>>>>     };
>>>>>
>>>>>     template <>
>>>>>     void A<0>::foo() {}
>>>>>
>>>>> In this example, the attribute is `final`, but it might also be an
>>>>> __attribute__((visibility("foo"))), noreturn, inline, etc. clang's
>>>>> current
>>>>> behavior is to strip all attributes, which for some attributes is wrong
>>>>> (the snippet above allows a subclass of A<0> to override the final
>>>>> method, for
>>>>> example) and for others disagrees with gcc
>>>>> (__attribute__((visibility()))).
>>>>>
>>>>> So stop dropping attributes. r95845 added this code without a test
>>>>> case, and
>>>>> r176728 added the code for dropping attributes on parameters (with
>>>>> tests, but
>>>>> they still pass).
>>>>>
>>>>> As an additional wrinkle, do drop dllimport and dllexport, since
>>>>> that's how these two
>>>>> attributes work. (This is covered by existing tests.)
>>>>>
>>>>> Fixes PR21942.
>>>>>
>>>>> The approach is by Richard Smith, initial analysis and typing was done
>>>>> by me.
>>>>>
>>>>
>>>> Naturally, this makes sense to me =) It also matches GCC and EDG on all
>>>> attributes I tested.
>>>>
>>>> John, do you remember why you added this code (a long long time ago)?
>>>>
>>>>
>>>> The behavior on attribute((visibility)) is intentional, I think.  At
>>>> the very least, you have to allow for explicit specializations to override
>>>> the visibility of the template, using a general most-specific-rule-wins
>>>> logic.
>>>>
>>>
>>> In practice, the attribute((visibility)) behavior seems unchanged,
>>> probably because it's propagated across redeclarations. I tried building
>>> the following program and compared `nm -m` output:
>>>
>>>     template <int>
>>>     struct A { void __attribute__((visibility("hidden"))) foo() {} };
>>>     template <> void __attribute__((visibility("default"))) A<0>::foo()
>>> {}
>>>
>>>     void f() {
>>>       A<0> a0; A<1> a1;
>>>       a0.foo(); a1.foo();
>>>     }
>>>
>>> I toggled each of the two attributes on and off, the nm output with and
>>> without the patch looked identical.
>>>
>>> I'm also doing a component build of Chromium with the patch, so far
>>> things look good.
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141219/52beff53/attachment.html>