Pickle: don't mark these methods inline when they're not

Pickle: don't mark these methods inline when they're not

BUG=none

[hans, 2014-05-22 17:34:06]

Please take a look.

[Nico, 2014-05-22 17:44:40]

Looking at https://codereview.chromium.org/34413002 , my guess at what this is
supposed to do:

Since these functions are out-of-line private template functions, they can only
be called from pickle.cc, and when that file is compiled the definitions are
available. So I'm guessing "inline" in that case is supposed to serve as a hint
to the compiler that these functions should be inlined.

(I'm not sure if "don't mark inline when they're not" makes sense even, as
inline class methods are implicitly marked "inline", so it's only needed for
out-of-line methods that have their definition available in all translation
units where they are used.)

[hans, 2014-05-22 18:03:52]

On 2014/05/22 17:44:40, Nico wrote:
> Looking at https://codereview.chromium.org/34413002 , my guess at what this is
> supposed to do:
> 
> Since these functions are out-of-line private template functions, they can
only
> be called from pickle.cc, and when that file is compiled the definitions are
> available. So I'm guessing "inline" in that case is supposed to serve as a
hint
> to the compiler that these functions should be inlined.

I guess I should have supplied more background :) This is for Clang's
-Wundefined-inline. That fires if the function is marked inline and is
considered used. With my brand new dllexport patch, those function are
considered used because we want to export them.

We could keep the 'inline' attributes in the .cc file, that should defy the
warning and still provide the inliner hint to compilers that rely on it.

[Nico, 2014-05-22 18:05:34]

On Thu, May 22, 2014 at 11:03 AM, <hans@chromium.org> wrote:

> On 2014/05/22 17:44:40, Nico wrote:
>
>> Looking at https://codereview.chromium.org/34413002 , my guess at what
>> this is
>> supposed to do:
>>
>
>  Since these functions are out-of-line private template functions, they can
>>
> only
>
>> be called from pickle.cc, and when that file is compiled the definitions
>> are
>> available. So I'm guessing "inline" in that case is supposed to serve as a
>>
> hint
>
>> to the compiler that these functions should be inlined.
>>
>
> I guess I should have supplied more background :) This is for Clang's
> -Wundefined-inline. That fires if the function is marked inline and is
> considered used.


How are they getting used in files other than pickle.cc?


> With my brand new dllexport patch, those function are
> considered used because we want to export them.
>
> We could keep the 'inline' attributes in the .cc file, that should defy the
> warning and still provide the inliner hint to compilers that rely on it.
>
> https://codereview.chromium.org/298913006/
>

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

[piman, 2014-05-22 18:15:24]

Please do not land this without checking for performance regressions and/or
generated code.

This is in the critical path every frame and was found necessary to tell the
compiler to consider these functions for inlining. They're only used when the
definition is available.

[hans, 2014-05-22 18:34:26]

On 2014/05/22 18:15:24, piman wrote:
> Please do not land this without checking for performance regressions and/or
> generated code.
> 
> This is in the critical path every frame and was found necessary to tell the
> compiler to consider these functions for inlining. They're only used when the
> definition is available.

Thanks for pointing this out. I'll make sure the generated code is identical
before landing anything.

[hans, 2014-05-22 18:41:02]

On 2014/05/22 18:05:34, Nico wrote:
> On Thu, May 22, 2014 at 11:03 AM, <mailto:hans@chromium.org> wrote:
> 
> > On 2014/05/22 17:44:40, Nico wrote:
> >
> >> Looking at https://codereview.chromium.org/34413002 , my guess at what
> >> this is
> >> supposed to do:
> >>
> >
> >  Since these functions are out-of-line private template functions, they can
> >>
> > only
> >
> >> be called from pickle.cc, and when that file is compiled the definitions
> >> are
> >> available. So I'm guessing "inline" in that case is supposed to serve as a
> >>
> > hint
> >
> >> to the compiler that these functions should be inlined.
> >>
> >
> > I guess I should have supplied more background :) This is for Clang's
> > -Wundefined-inline. That fires if the function is marked inline and is
> > considered used.
> 
> 
> How are they getting used in files other than pickle.cc?

They're not used per se, but since the function is dllexport, any file pulling
in that header is supposed to emit a definition, and we warn because it's not
defined.

I'm not saying that this warning means the code is ill-formed or anything, but
it struck me as confusing.

> (I'm not sure if "don't mark inline when they're not" makes sense even, as
inline class methods are implicitly marked "inline", so it's only needed for
out-of-line methods that have their definition available in all translation
units where they are used.)

It's only inline class method *definitions* that get implicitly marked inline.

[Nico, 2014-05-22 18:46:14]

On Thu, May 22, 2014 at 11:41 AM, <hans@chromium.org> wrote:

> On 2014/05/22 18:05:34, Nico wrote:
>
>  On Thu, May 22, 2014 at 11:03 AM, <mailto:hans@chromium.org> wrote:
>>
>
>  > On 2014/05/22 17:44:40, Nico wrote:
>> >
>> >> Looking at https://codereview.chromium.org/34413002 , my guess at what
>> >> this is
>> >> supposed to do:
>> >>
>> >
>> >  Since these functions are out-of-line private template functions, they
>> can
>> >>
>> > only
>> >
>> >> be called from pickle.cc, and when that file is compiled the
>> definitions
>> >> are
>> >> available. So I'm guessing "inline" in that case is supposed to serve
>> as a
>> >>
>> > hint
>> >
>> >> to the compiler that these functions should be inlined.
>> >>
>> >
>> > I guess I should have supplied more background :) This is for Clang's
>> > -Wundefined-inline. That fires if the function is marked inline and is
>> > considered used.
>>
>
>
>  How are they getting used in files other than pickle.cc?
>>
>
> They're not used per se, but since the function is dllexport, any file
> pulling
> in that header is supposed to emit a definition, and we warn because it's
> not
> defined.
>
> I'm not saying that this warning means the code is ill-formed or anything,
> but
> it struck me as confusing.
>
>
>  (I'm not sure if "don't mark inline when they're not" makes sense even, as
>>
> inline class methods are implicitly marked "inline", so it's only needed
> for
> out-of-line methods that have their definition available in all translation
> units where they are used.)
>
> It's only inline class method *definitions* that get implicitly marked
> inline.


(Right, my point is that explicitly marking methods inline only makes sense
for out-of-line methods, i.e. you only mark methods inline when they're not
:-) )

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

[hans, 2014-05-22 20:54:54]

After talking this through with my local language lawyer, we've decided that the
warning doesn't apply for dllexport (or dllimport) inline functions (fixed in
Clang r209471).