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

[Richard Smith]

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/db1d3711/attachment.html>