Clean-up inline members of nested classes (base/)

Clean-up inline members of nested classes (base/)

Due to a bug, the Clang-plugin style checker failed to warn about
inline constructors, destructors, non-empty virtual methods, etc.
for nested classes.

The plugin has been fixed, and this patch is part of a clean-up of all
the code that now causes the plugin to issue errors.

BUG=139346
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151016

[jar (doing other things), 2012-08-09 16:30:27]

https://chromiumcodereview.appspot.com/10825273/diff/1/base/system_monitor/sy...
File base/system_monitor/system_monitor.cc (right):

https://chromiumcodereview.appspot.com/10825273/diff/1/base/system_monitor/sy...
base/system_monitor/system_monitor.cc:53:
SystemMonitor::MediaDeviceInfo::MediaDeviceInfo(
nit: Since the declaration of this method in the nested class came before the
constructor in the surrounding class, the definition should be in that order.
(i.e., move this declaration to in front of line 25)

It seems a little weird... but they insisted on the type (and hence nested
classes) coming first, and so the corresponding definitions need to come first.

https://chromiumcodereview.appspot.com/10825273/diff/1/base/values.cc
File base/values.cc (right):

https://chromiumcodereview.appspot.com/10825273/diff/1/base/values.cc#newcode762
base/values.cc:762: : target_(target), it_(target.dictionary_.begin()) {}
nit: one initializer per line, now that you wrapped the line (despite the fact
that the header file had this wrong).

https://chromiumcodereview.appspot.com/10825273/diff/1/base/values.h
File base/values.h (right):

https://chromiumcodereview.appspot.com/10825273/diff/1/base/values.h#newcode343
base/values.h:343: : private std::iterator<std::input_iterator_tag, const
std::string> {
Any idea why this is using private inheritance (violating style guide)?  Do you
know how this functionality is used at all in the class?

https://chromiumcodereview.appspot.com/10825273/diff/1/base/values.h#newcode346
base/values.h:346: key_iterator(const key_iterator& rhs);
a) Why did you need to add this, if this CL is all about warnings?

b) Please add the explicit keyword.

[hans, 2012-08-09 18:28:48]

Thank you very much for the review! New patch uploaded.

https://chromiumcodereview.appspot.com/10825273/diff/1/base/system_monitor/sy...
File base/system_monitor/system_monitor.cc (right):

https://chromiumcodereview.appspot.com/10825273/diff/1/base/system_monitor/sy...
base/system_monitor/system_monitor.cc:53:
SystemMonitor::MediaDeviceInfo::MediaDeviceInfo(
On 2012/08/09 16:30:27, jar wrote:
> nit: Since the declaration of this method in the nested class came before the
> constructor in the surrounding class, the definition should be in that order.
> (i.e., move this declaration to in front of line 25)
> 
> It seems a little weird... but they insisted on the type (and hence nested
> classes) coming first, and so the corresponding definitions need to come
first.

Done.

https://chromiumcodereview.appspot.com/10825273/diff/1/base/values.cc
File base/values.cc (right):

https://chromiumcodereview.appspot.com/10825273/diff/1/base/values.cc#newcode762
base/values.cc:762: : target_(target), it_(target.dictionary_.begin()) {}
On 2012/08/09 16:30:27, jar wrote:
> nit: one initializer per line, now that you wrapped the line (despite the fact
> that the header file had this wrong).

Done.

https://chromiumcodereview.appspot.com/10825273/diff/1/base/values.h
File base/values.h (right):

https://chromiumcodereview.appspot.com/10825273/diff/1/base/values.h#newcode343
base/values.h:343: : private std::iterator<std::input_iterator_tag, const
std::string> {
On 2012/08/09 16:30:27, jar wrote:
> Any idea why this is using private inheritance (violating style guide)?  Do
you
> know how this functionality is used at all in the class?

No idea, I haven't really looked at the file itself, this is just a mechanical
change.

https://chromiumcodereview.appspot.com/10825273/diff/1/base/values.h#newcode346
base/values.h:346: key_iterator(const key_iterator& rhs);
On 2012/08/09 16:30:27, jar wrote:
> a) Why did you need to add this, if this CL is all about warnings?
Because the plugin warns that this class needs an out-of-line copy constructor.

If we don't add one, the compiler will synthesize a default one for us, which is
inline, and the style guide tries to limit the amount of complex inline
functions in header files.

> b) Please add the explicit keyword.

I don't think we do that for copy constructors.

[jar (doing other things), 2012-08-09 19:27:16]

With nit (comment)....

LGTM

https://chromiumcodereview.appspot.com/10825273/diff/1/base/values.h
File base/values.h (right):

https://chromiumcodereview.appspot.com/10825273/diff/1/base/values.h#newcode346
base/values.h:346: key_iterator(const key_iterator& rhs);
You are correct about explicit. Thanks!

I think the style guide asks for a comment to that effect (noting why it is an
exception).




On 2012/08/09 18:28:48, hans wrote:
> On 2012/08/09 16:30:27, jar wrote:
> > a) Why did you need to add this, if this CL is all about warnings?
> Because the plugin warns that this class needs an out-of-line copy
constructor.
> 
> If we don't add one, the compiler will synthesize a default one for us, which
is
> inline, and the style guide tries to limit the amount of complex inline
> functions in header files.
> 
> > b) Please add the explicit keyword.
> 
> I don't think we do that for copy constructors.

[hans, 2012-08-09 19:31:05]

Thanks!

https://chromiumcodereview.appspot.com/10825273/diff/1/base/values.h
File base/values.h (right):

https://chromiumcodereview.appspot.com/10825273/diff/1/base/values.h#newcode346
base/values.h:346: key_iterator(const key_iterator& rhs);
On 2012/08/09 19:27:16, jar wrote:
> You are correct about explicit. Thanks!
> 
> I think the style guide asks for a comment to that effect (noting why it is an
> exception).

Comment added.

[commit-bot: I haz the power, 2012-08-09 19:31:42]

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

[commit-bot: I haz the power, 2012-08-09 19:53:47]

Try job failure for 10825273-6005 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-09 20:05:36]

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

[commit-bot: I haz the power, 2012-08-09 21:01:01]

Try job failure for 10825273-2003 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-10 07:25:12]

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

[commit-bot: I haz the power, 2012-08-10 08:59:01]

Try job failure for 10825273-14001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-10 09:02:36]

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

[commit-bot: I haz the power, 2012-08-10 12:45:36]

Change committed as 151016