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 <[email protected]> wrote: > > On Fri, Dec 19, 2014 at 12:58 PM, Reid Kleckner <[email protected]> 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 <[email protected]> wrote: >> >>> On Thu, Dec 18, 2014 at 9:58 PM, John McCall <[email protected]> wrote: >>>> >>>> On Dec 18, 2014, at 6:18 PM, Richard Smith <[email protected]> >>>> wrote: >>>> On Wed, Dec 17, 2014 at 6:58 PM, Nico Weber <[email protected]> >>>> 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 >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >>>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
