dllexport Pickle::WriteBytesStatic

dllexport Pickle::WriteBytesStatic

WriteBytesStatic is called by methods defined in pickle.h (e.g. WriteInt,
which calls WritePod<4>). If those methods are inlined (MSVC doesn't seem
to do that, but Clang does), the definition of WriteBytesStatic needs to
be available across the dll boundry.

(Note that dllimport/export isn't inherited by class member templates,
but the visibility attribute that we use on other platforms is.)

BUG=82385
TEST=build ipc.dll with Clang in a shared_library Release build
R=piman@chromium.org, rnk@chromium.org, thakis@chromium.org

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

[hans, 2014-06-09 20:44:44]

Please take a look!

[Reid Kleckner, 2014-06-09 21:17:15]

On 2014/06/09 20:44:44, hans wrote:
> Please take a look!

pickle.h is activating my "this C++ isn't well-formed" senses, but I'm not
enough of a lawyer to say for sure.  WriteBytesCommon is declared inline, but
not defined in a header.  The template is declared, but not defined.  The code
may be well-formed, but it's asking for trouble.  Can we hoist the template
definition, but declare some extern explicit instantiations that get provided by
the .cc file?

[Reid Kleckner, 2014-06-09 21:43:17]

On 2014/06/09 21:17:15, Reid Kleckner wrote:
> On 2014/06/09 20:44:44, hans wrote:
> > Please take a look!
> 
> pickle.h is activating my "this C++ isn't well-formed" senses, but I'm not
> enough of a lawyer to say for sure.  WriteBytesCommon is declared inline, but
> not defined in a header.  The template is declared, but not defined.  The code
> may be well-formed, but it's asking for trouble.  Can we hoist the template
> definition, but declare some extern explicit instantiations that get provided
by
> the .cc file?

Thinking about it more, this doesn't make the situation any worse, and I don't
want to accidentally change performance.  Don't hold this up on my account.

lgtm

[piman, 2014-06-10 00:57:00]

LGTM

[hans, 2014-06-10 00:59:31]

The CQ bit was checked by hans@chromium.org

[commit-bot: I haz the power, 2014-06-10 01:00:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hans@chromium.org/327613002/1

[Nico, 2014-06-10 16:04:03]

Do you know why this isn't needed on non-windows?

[Nico, 2014-06-10 16:09:58]

I suppose the rules for dllexport and explicit visibility attributes are just
different?

…and in other places where we have explicit template instantiation declarations
(e.g. base/strings/string16.h / cc), we put the export on the declaration, not
the definition. Should the export be on the declaration in the header instead?

(It's not completely the same, as this here is a member function template, not a
full class like in string.)

[hans, 2014-06-10 16:34:31]

On 2014/06/10 16:09:58, Nico (away) wrote:
> I suppose the rules for dllexport and explicit visibility attributes are just
> different?

Yes.

dllexport/dllimport does not get inherited by member template functions or
nested classes. The Posix visibility attribute seems to behave in the more
expected way, and does get inherited.

> …and in other places where we have explicit template instantiation
declarations
> (e.g. base/strings/string16.h / cc), we put the export on the declaration, not
> the definition. Should the export be on the declaration in the header instead?
> 
> (It's not completely the same, as this here is a member function template, not
a
> full class like in string.)

Hmm, yes, we should put it on the declaration.

Actually, I'm not sure how this even worked before.. how did writePOD know that
WriteBytesStatic should be imported?

[hans, 2014-06-10 16:38:03]

New patch uploaded.

[Nico, 2014-06-10 16:40:16]

patch set 2 lgtm

[hans, 2014-06-10 16:41:07]

The CQ bit was checked by hans@chromium.org

[commit-bot: I haz the power, 2014-06-10 16:46:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hans@chromium.org/327613002/20001

[chromium-reviews, 2014-06-10 17:33:26]

On Tue, Jun 10, 2014 at 9:34 AM, <hans@chromium.org> wrote:
>
> Hmm, yes, we should put it on the declaration.
>

sgtm


> Actually, I'm not sure how this even worked before.. how did writePOD know
> that
> WriteBytesStatic should be imported?
>

You don't actually need dllimport for functions.  The import library will
provide forwarding thunks for functions, but it can't do that for data.

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

[commit-bot: I haz the power, 2014-06-11 00:20:05]

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

[hans, 2014-06-11 00:34:31]

On 2014/06/11 00:20:05, I haz the power (commit-bot) wrote:
> FYI, CQ is re-trying this CL (attempt #1).
> The failing builders are:
>   win_chromium_x64_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

It's failing with and without the patch, and in other tryjobs than mine. I'll
land manually.

[hans, 2014-06-11 00:34:43]

Committed patchset #2 manually as r276218 (presubmit successful).