RefCounted types should not have public destructors, chrome/browser/ part 6

RefCounted types should not have public destructors, chrome/browser/ part 6

BUG=123295
TEST=none


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

[Ryan Sleevi, 2012-04-13 09:14:12]

pkasting: PTAL

[Peter Kasting, 2012-04-13 09:53:03]

First: Thanks for doing this annoying but useful transform.

Next, I really hate to say this but -- if you're going to move the declaration
order in the header, you should change the definition order in the C++ file to
match.  Maybe somehow you can automate that process?  I dunno :(.  I'm also not
happy about either forcing you to do this in your gazillion other patches, or
just letting a whole ton of C++ files now have mismatching function orders :(.

(Tiny nit: I'm also not a huge fan of putting the function braces on the same
line, a la "Foo::~Foo() {}", when it's an out-of-line definition; I normally
reserve that for inline definitions in headers.)

[Ryan Sleevi, 2012-04-16 18:49:04]

On 2012/04/13 09:53:03, Peter Kasting wrote:
> First: Thanks for doing this annoying but useful transform.
> 
> Next, I really hate to say this but -- if you're going to move the declaration
> order in the header, you should change the definition order in the C++ file to
> match.  Maybe somehow you can automate that process?  I dunno :(.  I'm also
not
> happy about either forcing you to do this in your gazillion other patches, or
> just letting a whole ton of C++ files now have mismatching function orders :(.

The push-back I got from other OWNERS indicate a concern with code-churn & blame
churn this might cause - as previously seen when erg@ attempted this.

Given that erg@'s plugin was never checked in, this is a style element that
hasn't been consistently enforced and unfortunately regressed. While not trying
to argue against doing this (I'm a big fan of it), I also would prefer not to
have potential bug checks hung up on style issues.

Since there was a plugin (at one point) that warned about this inconsistency,
would you be amenable to not requiring the ordering for this commit, and then
saving it for a later commit? With automation, we can cover everything but
Windows files.

> (Tiny nit: I'm also not a huge fan of putting the function braces on the same
> line, a la "Foo::~Foo() {}", when it's an out-of-line definition; I normally
> reserve that for inline definitions in headers.)

I've gotten dinged by reviewers who take the opposite approach, hence the bits.

In http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml , all of the
examples of empty bodies use {}. The exception to this is constructor
initializer lists that cover multiple lines (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...
), which is internally consistent with
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Also seems to fit with the guidance of
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Vertic...
on minimizing vertical whitespace

[Peter Kasting, 2012-04-16 19:30:06]

On 2012/04/16 18:49:04, Ryan Sleevi wrote:
> On 2012/04/13 09:53:03, Peter Kasting wrote:
> > Next, I really hate to say this but -- if you're going to move the
declaration
> > order in the header, you should change the definition order in the C++ file
to
> > match.  Maybe somehow you can automate that process?  I dunno :(.  I'm also
> not
> > happy about either forcing you to do this in your gazillion other patches,
or
> > just letting a whole ton of C++ files now have mismatching function orders
:(.
> 
> The push-back I got from other OWNERS indicate a concern with code-churn &
blame
> churn this might cause - as previously seen when erg@ attempted this.

I don't see how that concern is lessened by doing it later.  If we're moving the
code, we're moving the code.  The only way this doesn't cause churn is by not
doing it at all, and whether or not this is sufficiently enforced, it is a style
rule that I don't want to intentionally regress.

I also disagree pretty strongly with people who want to minimize code churn at
the expense of style rules.  We shouldn't churn for no reason, but this is
hardly no reason.  And we have so much code churn from legitimate sources that
anyone who isn't capable of tracing things back through a couple levels of svn
annotate (or whatever) is going to be frustrated anyway.

> Since there was a plugin (at one point) that warned about this inconsistency,
> would you be amenable to not requiring the ordering for this commit, and then
> saving it for a later commit? With automation, we can cover everything but
> Windows files.

I would prefer to keep the .cc reordering in the same changes that modify the .h
ordering -- it's much easier to understand when and why we're doing this in that
case.

In the limit I am OK with splitting this into two changes but only if you are
actually going to make the .cc changes immediately after you finish the .h
changes.  I'm not OK with "punt to unspecified future date".

I'm willing to argue my position on the Chromium mailing lists if need be.  If
there's a lot of OWNERS disagreement then we should perhaps be on the same page
before a ton of this stuff lands.

[Ryan Sleevi, 2012-04-27 00:05:56]

Peter, please take a second look.

While taking into consideration the nit (re {} vs { \n }), I believe in the
spirit of
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...
that it's more consistent with the style guide to use the single line form for
destructors.

Having now gone through several hundred files addressing the ordering issue, I
also believe it's more consistent with Chromium code. I hope you can forgive me
for taking that small bit of consistency matching.

Please also note that protocol handler registry was updated to have definitions
match declarations. Given how many files were already consistent, and given that
I already had to search through the file to make a 'best effort' as to where to
place the destructor, it was just worth it to fix the dang thing.

[Peter Kasting, 2012-04-27 00:08:40]

LGTM.  I didn't review each file; I'm fine with {} on one line for destructors.

[commit-bot: I haz the power, 2012-04-27 00:39:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10071036/5001

[commit-bot: I haz the power, 2012-04-27 02:56:37]

Change committed as 134218