dbus: don't add attributes in extern template class specialization

dbus: don't add attributes in extern template class specialization

This previous CL added attributes to some template specializations:
https://codereview.chromium.org/408143012

This breaks gcc 4.8, which gives the following warning:
  ../../dbus/property.h:412:62: error: type attributes ignored after type is already defined [-Werror=attributes]

Since the template class already has the attribute, I suspect that the
additional attribute specifications are redundant.  ie I think we can
remove them.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286567

[Mostyn Bramley-Moore, 2014-07-29 22:43:47]

@tzik: does this small patch still work for non-ChromeOS build + latest clang?

[Nico, 2014-07-29 22:46:04]

I think this is the right fix, see
https://codereview.chromium.org/419203003/#msg5

But tzik said on that thread that it didn't work under some circumstances?

I guess if the bots are happy, it's fine, so lgtm.

(see also https://codereview.chromium.org/424453003 ;-) )

[Mostyn Bramley-Moore, 2014-07-29 22:58:16]

On 2014/07/29 22:46:04, Nico (away) wrote:
> I think this is the right fix, see
> https://codereview.chromium.org/419203003/#msg5
> 
> But tzik said on that thread that it didn't work under some circumstances?

;) I'll check clang+asan.

[Nico, 2014-07-29 22:59:27]

On Tue, Jul 29, 2014 at 3:58 PM, <mostynb@opera.com> wrote:

> On 2014/07/29 22:46:04, Nico (away) wrote:
>
>> I think this is the right fix, see
>> https://codereview.chromium.org/419203003/#msg5
>>
>
>  But tzik said on that thread that it didn't work under some circumstances?
>>
>
> ;) I'll check clang+asan.


I tested that already (see link above), it worked for me.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tzik, 2014-07-30 05:13:44]

On 2014/07/29 22:59:27, Nico (away) wrote:
> On Tue, Jul 29, 2014 at 3:58 PM, <mailto:mostynb@opera.com> wrote:
> 
> > On 2014/07/29 22:46:04, Nico (away) wrote:
> >
> >> I think this is the right fix, see
> >> https://codereview.chromium.org/419203003/#msg5
> >>
> >
> >  But tzik said on that thread that it didn't work under some circumstances?
> >>
> >
> > ;) I'll check clang+asan.
> 
> 
> I tested that already (see link above), it worked for me.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Hm, seems like it fails without CHROME_DBUS_EXPORT only between r284713 and
r286068 + clang + asan=1 + component=shared_library.
Now, it works for me.

LGTM.

[tzik, 2014-07-30 05:15:43]

On 2014/07/30 05:13:44, tzik wrote:
> On 2014/07/29 22:59:27, Nico (away) wrote:
> > On Tue, Jul 29, 2014 at 3:58 PM, <mailto:mostynb@opera.com> wrote:
> > 
> > > On 2014/07/29 22:46:04, Nico (away) wrote:
> > >
> > >> I think this is the right fix, see
> > >> https://codereview.chromium.org/419203003/#msg5
> > >>
> > >
> > >  But tzik said on that thread that it didn't work under some
circumstances?
> > >>
> > >
> > > ;) I'll check clang+asan.
> > 
> > 
> > I tested that already (see link above), it worked for me.
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Hm, seems like it fails without CHROME_DBUS_EXPORT only between r284713 and
> r286068 + clang + asan=1 + component=shared_library.
> Now, it works for me.
> 
> LGTM.

CCing vivekg@.

[Mostyn Bramley-Moore, 2014-07-30 07:42:53]

The CQ bit was checked by mostynb@opera.com

[commit-bot: I haz the power, 2014-07-30 07:44:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mostynb@opera.com/423233003/1

[commit-bot: I haz the power, 2014-07-30 08:59:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-07-30 09:03:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-30 09:03:20]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Mostyn Bramley-Moore, 2014-07-30 09:38:58]

Oops, missing OWNER approval.

@keybuk: can you please take a look?

[keybuk, 2014-07-30 16:50:30]

On 2014/07/30 09:38:58, Mostyn Bramley-Moore wrote:
> Oops, missing OWNER approval.
> 
> @keybuk: can you please take a look?

I no longer understand what you're doing here, or why you think this works - to
me this looks like you're claiming that they'll be a global instantiation of the
property specialization whether or not it's going to be used

This doesn't seem right

[Nico, 2014-07-30 16:51:46]

On 2014/07/30 16:50:30, keybuk wrote:
> On 2014/07/30 09:38:58, Mostyn Bramley-Moore wrote:
> > Oops, missing OWNER approval.
> > 
> > @keybuk: can you please take a look?
> 
> I no longer understand what you're doing here, or why you think this works -
to
> me this looks like you're claiming that they'll be a global instantiation of
the
> property specialization whether or not it's going to be used
> 
> This doesn't seem right

The specializations take the visibility from the general template.

We tested this on all platforms we could think of, it does work in practice.
It's also how this pattern is used in other places in the code base (e.g.
base/strings/string16.h)

I'm pretty sure it's correct.

[keybuk, 2014-07-30 16:56:52]

On 2014/07/30 16:51:46, Nico (away) wrote:
> I'm pretty sure it's correct.

If you're sure it's correct, then go ahead and commit it

[Nico, 2014-07-30 16:58:50]

On 2014/07/30 16:56:52, keybuk wrote:
> On 2014/07/30 16:51:46, Nico (away) wrote:
> > I'm pretty sure it's correct.
> 
> If you're sure it's correct, then go ahead and commit it

Can you lgtm then, please? Else the cq can't land it.

[keybuk, 2014-07-30 17:07:41]

On 2014/07/30 16:58:50, Nico (away) wrote:
> On 2014/07/30 16:56:52, keybuk wrote:
> > On 2014/07/30 16:51:46, Nico (away) wrote:
> > > I'm pretty sure it's correct.
> > 
> > If you're sure it's correct, then go ahead and commit it
> 
> Can you lgtm then, please? Else the cq can't land it.

Nope, it doesn't to me - I have no idea what you're doing, and I can't rubber
stump patches I don't understand

[Nico, 2014-07-30 17:21:13]

On 2014/07/30 17:07:41, keybuk wrote:
> On 2014/07/30 16:58:50, Nico (away) wrote:
> > On 2014/07/30 16:56:52, keybuk wrote:
> > > On 2014/07/30 16:51:46, Nico (away) wrote:
> > > > I'm pretty sure it's correct.
> > > 
> > > If you're sure it's correct, then go ahead and commit it
> > 
> > Can you lgtm then, please? Else the cq can't land it.
> 
> Nope, it doesn't to me - I have no idea what you're doing, and I can't rubber
> stump patches I don't understand

It is a keywords the cq looks for. Pretend it means with "I'm fine with you
landing this", not the literal "looks good to me"

[Mostyn Bramley-Moore, 2014-07-30 17:32:20]

To try and explain the patch a bit more:

CHROME_DBUS_EXPORT is a macro which in component=shared_library builds expands
to __attribute__((visibility("default"))).  When specified for something
corresponding to a symbol in the resulting shared library, the symbol is
exported and usable from code outside the library.

The Property template class is defined with CHROME_DBUS_EXPORT, so it's visible
in component builds:
template <class T>
class CHROME_DBUS_EXPORT Property : public PropertyBase {
...

Then the specialization, eg of Property<uint8> was also defined with
CHROME_DBUS_EXPORT:
extern template class CHROME_DBUS_EXPORT Property<uint8>;

GCC complains that the attribute from CHROME_DBUS_EXPORT is being ignored in the
"extern template" statement:
../../dbus/property.h:412:62: error: type attributes ignored after type is
already defined [-Werror=attributes]

This patch simply removes the attribute from the specialization, but leaves it
in the template class definition.  The compiler still marks the entire template
class as visible.  If this patch were to break anything, it would be linking
dbus_unittests in component=shared_library builds- but we have tested that and
seen that it works.

[satorux1, 2014-07-30 18:36:08]

LGTM. mostynb@, thank you for explaining this.

[Nico, 2014-07-30 18:37:07]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-07-30 18:38:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mostynb@opera.com/423233003/1

[commit-bot: I haz the power, 2014-07-30 18:44:45]

Change committed as 286567