[NOT COMMITTED] Inline TaskTraits' destructor per it only having POD type members

Inline TaskTraits' destructor per it only having POD type members

BUG=553459

[gab, 2016-04-07 17:37:46]

gab@chromium.org changed reviewers:
+ robliao@chromium.org

[gab, 2016-04-07 17:37:46]

Rob makes sense to you? (CC fdoray, simple CL doesn't require 2 reviewers)

[robliao, 2016-04-07 17:43:18]

https://codereview.chromium.org/1868933003/diff/1/base/task_scheduler/task_tr...
File base/task_scheduler/task_traits.h (right):

https://codereview.chromium.org/1868933003/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_traits.h:90: ~TaskTraits() = default;
The Chromium guidance is

"Define them in the implementation file unless you really need them to be
inlined."

Also noted, this also apparently breaks forward declared variables (presumably
since the compiler now needs to know everything about setting the object up). 

https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i...

Having said that, do we gain something from inlining this destructor?

[gab, 2016-04-08 21:00:04]

https://codereview.chromium.org/1868933003/diff/1/base/task_scheduler/task_tr...
File base/task_scheduler/task_traits.h (right):

https://codereview.chromium.org/1868933003/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_traits.h:90: ~TaskTraits() = default;
On 2016/04/07 17:43:18, robliao wrote:
> The Chromium guidance is
> 
> "Define them in the implementation file unless you really need them to be
> inlined."
> 
> Also noted, this also apparently breaks forward declared variables (presumably
> since the compiler now needs to know everything about setting the object up). 

Hmm, interesting, do we expect TaskTraits to be fwd-declared much?

> 
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i...
> 
> Having said that, do we gain something from inlining this destructor?

If it's not inlined then SchedulerParallelTaskRunner also has to have an
explicit out-of-line destructor:
https://codereview.chromium.org/1708773002/diff/550001/base/task_scheduler/sc...

(that review is why I wondered whether TaskTraits had a trivial inline
destructor with POD types only)

But I guess the point of that style rule is that having that dependency on
SchedulerParallelTaskRunner is no good anyways (probably doesn't matter much in
that case since all callers won't inline but rather use virtual dispatch to get
to the destructor but wtv).

So I guess not inlining is the right thing here after all.

Closing, thanks.

[gab, 2016-04-08 21:00:16]

Description was changed from

==========
Inline TaskTraits' destructor per it only having POD type members

BUG=553459
==========

to

==========
Inline TaskTraits' destructor per it only having POD type members

BUG=553459
==========

[gab, 2016-04-08 21:01:48]

https://codereview.chromium.org/1868933003/diff/1/base/task_scheduler/task_tr...
File base/task_scheduler/task_traits.h (right):

https://codereview.chromium.org/1868933003/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_traits.h:90: ~TaskTraits() = default;
On 2016/04/08 21:00:04, gab wrote:
> On 2016/04/07 17:43:18, robliao wrote:
> > The Chromium guidance is
> > 
> > "Define them in the implementation file unless you really need them to be
> > inlined."
> > 
> > Also noted, this also apparently breaks forward declared variables
(presumably
> > since the compiler now needs to know everything about setting the object
up). 
> 
> Hmm, interesting, do we expect TaskTraits to be fwd-declared much?
> 
> > 
> >
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i...
> > 
> > Having said that, do we gain something from inlining this destructor?
> 
> If it's not inlined then SchedulerParallelTaskRunner also has to have an
> explicit out-of-line destructor:
>
https://codereview.chromium.org/1708773002/diff/550001/base/task_scheduler/sc...
> 
> (that review is why I wondered whether TaskTraits had a trivial inline
> destructor with POD types only)
> 
> But I guess the point of that style rule is that having that dependency on
> SchedulerParallelTaskRunner is no good anyways (probably doesn't matter much
in
> that case since all callers won't inline but rather use virtual dispatch to
get
> to the destructor but wtv).

Actually that destructor is private so virtual dispatch is really the only way
to get to it and inlining isn't a concern, so nothing needs to change, ignore me
:-)!

> 
> So I guess not inlining is the right thing here after all.
> 
> Closing, thanks.