Add virtual and OVERRIDE to base/ implementation files

Add virtual and OVERRIDE to base/ implementation files

BUG=none
TEST=compiles


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

[Ryan Sleevi, 2012-04-05 05:27:41]

thakis: Before I get a base/ reviewer, WDYT about changes like this?

I have a hacked up change to the style checker plugin to warn on implementation
files if they're missing virtual+OVERRIDE, but not about out-of-line virtuals or
the heavy ctor/dtor.

My main motivation was to enable the style checking for RCTS in implementation
files, since that's where the bulk of the changes were from
http://codereview.chromium.org/9958145/ , but it seems quite useful to also have
"virtual" (style nit) and OVERRIDE (can catch real bugs).

I could exclude unit tests altogether (by doing something as silly as matching
_unittest in the filename, which isn't perfect but would catch most of the
errors). However, it seems better to standardize style rather than having prod
code/test code split.

Thoughts on this approach?

If you agree with it, I seem to recall that either your or akalin had a source
translator/rewriter for some of the base::Bind/Callback stuff. Seems it would be
quite useful here, since this is mostly mechanical translation. Otherwise, it's
a "while I'm watching TV" sort of activity.

[Nico, 2012-04-05 05:30:39]

I think this is reasonable. But it's probably best if you reply to Avi's "should
we make OVERRIDE mandatory?" thread and just ask if people think that making it
mandatory in cc files is cool too. Since everybody was supportive in header
files, that is probably the case.

[Avi (use Gerrit), 2012-04-05 14:59:40]

On 2012/04/05 05:30:39, Nico wrote:
> I think this is reasonable. But it's probably best if you reply to Avi's
"should
> we make OVERRIDE mandatory?" thread and just ask if people think that making
it
> mandatory in cc files is cool too. Since everybody was supportive in header
> files, that is probably the case.

My thread asked if we should make OVERRIDE mandatory; it didn't talk about
header files vs implementation ones. The fact that the checker doesn't catch
those is an oversight on my part.

[Nico, 2012-04-05 17:52:34]

https://chromiumcodereview.appspot.com/10004001/diff/1/base/metrics/stats_tab...
File base/metrics/stats_table_unittest.cc (right):

https://chromiumcodereview.appspot.com/10004001/diff/1/base/metrics/stats_tab...
base/metrics/stats_table_unittest.cc:80: virtual ~StatsTableThread() {}
Why is this needed?

[Ryan Sleevi, 2012-04-05 19:12:07]

jar: base/ ready for review. Looks like no detractors from the chromium-dev
thread (so far), so it seems reasonable to push for review.

http://codereview.chromium.org/10004001/diff/1/base/metrics/stats_table_unitt...
File base/metrics/stats_table_unittest.cc (right):

http://codereview.chromium.org/10004001/diff/1/base/metrics/stats_table_unitt...
base/metrics/stats_table_unittest.cc:80: virtual ~StatsTableThread() {}
On 2012/04/05 17:52:34, Nico wrote:
> Why is this needed?

D'oh! It isn't. This crept in when I was using an earlier (bugged) set of
changes to the Clang plugin. Thanks for catching.

[Nico, 2012-04-05 19:16:02]

lgtm, but i'm not in base/owners

http://codereview.chromium.org/10004001/diff/1033/base/synchronization/waitab...
File base/synchronization/waitable_event_watcher_posix.cc (right):

http://codereview.chromium.org/10004001/diff/1033/base/synchronization/waitab...
base/synchronization/waitable_event_watcher_posix.cc:46: friend class
RefCountedThreadSafe<Flag>;
likely unnecessary?

http://codereview.chromium.org/10004001/diff/1033/base/synchronization/waitab...
base/synchronization/waitable_event_watcher_posix.cc:47: ~Flag() {}
unnecessary, or at least unrelated?

[Ryan Sleevi, 2012-04-05 19:22:37]

http://codereview.chromium.org/10004001/diff/1033/base/synchronization/waitab...
File base/synchronization/waitable_event_watcher_posix.cc (right):

http://codereview.chromium.org/10004001/diff/1033/base/synchronization/waitab...
base/synchronization/waitable_event_watcher_posix.cc:47: ~Flag() {}
On 2012/04/05 19:16:02, Nico wrote:
> unnecessary, or at least unrelated?

Unnecessary no, unrelated yes.

This bled in from http://codereview.chromium.org/9997007/ on my git branch. Will
shift it over to that CL.

[jar (doing other things), 2012-04-05 21:11:30]

Since you're in there fixing things... I pointed out adjacent stuff that should
probably be cleaned up.  The nits wouldn't be worth it to do alone... but you
may as well do them with this cleaning change IMO.

With nits fixed, LGTM for base.

http://codereview.chromium.org/10004001/diff/1033/base/memory/weak_ptr_unitte...
File base/memory/weak_ptr_unittest.cc (right):

http://codereview.chromium.org/10004001/diff/1033/base/memory/weak_ptr_unitte...
base/memory/weak_ptr_unittest.cc:48: : Thread("owner_thread") {
nit: You may as well put this all on one line while cleaning. (including
trailing curly)

http://codereview.chromium.org/10004001/diff/1033/base/metrics/stats_table_un...
File base/metrics/stats_table_unittest.cc (right):

http://codereview.chromium.org/10004001/diff/1033/base/metrics/stats_table_un...
base/metrics/stats_table_unittest.cc:79: id_(id) {}
nit: indent by 2 more spaces.

http://codereview.chromium.org/10004001/diff/1033/base/observer_list_unittest.cc
File base/observer_list_unittest.cc (right):

http://codereview.chromium.org/10004001/diff/1033/base/observer_list_unittest...
base/observer_list_unittest.cc:42: : list_(list), doomed_(doomed) { }
nit: one initializer per line.

Also the occaisonal use of spaces between curlies like:
{ }
may as well be cleaned to:
{}
(which is used mostly in this file... but not all the time :-(.  )

http://codereview.chromium.org/10004001/diff/1033/base/process_util_unittest.cc
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/10004001/diff/1033/base/process_util_unittest....
base/process_util_unittest.cc:943: #if defined(USE_TCMALLOC)
nit: can we move line 943 to 941, and line 949 to line 951?

This ifdef crossing declaration boundaries are really painful.

http://codereview.chromium.org/10004001/diff/1033/base/threading/thread_local...
File base/threading/thread_local_unittest.cc (right):

http://codereview.chromium.org/10004001/diff/1033/base/threading/thread_local...
base/threading/thread_local_unittest.cc:20: : tlp_(tlp), done_(done) { }
nit: one initializer per line.

http://codereview.chromium.org/10004001/diff/1033/base/threading/thread_local...
base/threading/thread_local_unittest.cc:31: : ThreadLocalTesterBase(tlp, done),
val_(NULL) { }
nit: one init per line.

http://codereview.chromium.org/10004001/diff/1033/base/threading/thread_unitt...
File base/threading/thread_unittest.cc (right):

http://codereview.chromium.org/10004001/diff/1033/base/threading/thread_unitt...
base/threading/thread_unittest.cc:69: : Thread("none"), event_list_(event_list)
{
nit: one init per line.

http://codereview.chromium.org/10004001/diff/1033/base/threading/watchdog_uni...
File base/threading/watchdog_unittest.cc (right):

http://codereview.chromium.org/10004001/diff/1033/base/threading/watchdog_uni...
base/threading/watchdog_unittest.cc:25: : Watchdog(duration,
thread_watched_name, enabled), alarm_counter_(0) {
nit: one init per line.

[commit-bot: I haz the power, 2012-04-05 21:59:57]

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

[commit-bot: I haz the power, 2012-04-05 22:40:23]

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

[commit-bot: I haz the power, 2012-04-06 00:56:33]

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

[commit-bot: I haz the power, 2012-04-06 03:53:05]

Change committed as 131098