Replace most LOG statements with DLOG statements in base.

Replace most LOG/CHECK statements with DLOG/DCHECK statements in base.

I tried hard not to change CHECKs that had side effects. I kept fatal checks
that seemed security or debugging-info (in crash reports) sensitive, and ones
that seems particularly well-conceived.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107042

[brettw, 2011-10-21 21:26:06]

This is not just a search and replace. In particular, we need to be wary about
changing ones with side effects or places where we want to crash for security or
crash stat reasons.

[jar (doing other things), 2011-10-23 00:36:28]

Nice pass on a lot of files.  I made a bunch of comments below.

Thanks,

Jim

http://codereview.chromium.org/8368009/diff/6017/base/debug/trace_event_unitt...
File base/debug/trace_event_unittest.cc (right):

http://codereview.chromium.org/8368009/diff/6017/base/debug/trace_event_unitt...
base/debug/trace_event_unittest.cc:104: NOTREACHED();
This is a unit test... so CHECK vs DCHECK is not critical... but if anything, it
should be a flavor of ASSERT.

This should probably be an ASSERT_TRUE(false);

http://codereview.chromium.org/8368009/diff/6017/base/dir_reader_posix_unitte...
File base/dir_reader_posix_unittest.cc (right):

http://codereview.chromium.org/8368009/diff/6017/base/dir_reader_posix_unitte...
base/dir_reader_posix_unittest.cc:30: DCHECK(dir);
better would be an ASSERT (in a unit test)

http://codereview.chromium.org/8368009/diff/6017/base/message_pump_glib.cc
File base/message_pump_glib.cc (right):

http://codereview.chromium.org/8368009/diff/6017/base/message_pump_glib.cc#ne...
base/message_pump_glib.cc:148: DCHECK_EQ(pipe(fds), 0);
This needs to be done for the side-effect of loading the file-descriptor array
(2 elements):
int ret = pipe(fds);
DCHECK_EQ(ret, 0);

http://codereview.chromium.org/8368009/diff/6017/base/pickle.cc
File base/pickle.cc (right):

http://codereview.chromium.org/8368009/diff/6017/base/pickle.cc#newcode404
base/pickle.cc:404: DCHECK_NE(capacity_, kCapacityReadOnly);
I've had a lot of trouble with this class and memory over-writes.  This is also
a central security area (blocking corrupt un-pickles).  I'd rather see this one
stay as a CHECK.

http://codereview.chromium.org/8368009/diff/6017/base/rand_util_posix.cc
File base/rand_util_posix.cc (right):

http://codereview.chromium.org/8368009/diff/6017/base/rand_util_posix.cc#newc...
base/rand_util_posix.cc:52: DCHECK(success);
I think this is worth leaving as a CHECK.  Random numbers are precious to SSL
etc. negotiations, and hence pivotal to security.

http://codereview.chromium.org/8368009/diff/6017/base/synchronization/waitabl...
File base/synchronization/waitable_event_win.cc (right):

http://codereview.chromium.org/8368009/diff/6017/base/synchronization/waitabl...
base/synchronization/waitable_event_win.cc:78: DCHECK_LE(count,
MAXIMUM_WAIT_OBJECTS)
This protects us from a buffer overrun if we could be fooled into having too
many waitable events :-/.  I also wouldn't be surprised is some dynamic set of
events led us to violating this restriction some day.  I think CHECK() might be
good.

If you want to yank the CHECK, you need something (at least) like:
count = std::min(count, MAXIMUM_WAIT_OBJECTS);

...but even then... it varies the semantics by not waiting for some of the
supplied items :-(.  Again, crashing might be better.

http://codereview.chromium.org/8368009/diff/6017/base/test/test_timeouts.cc
File base/test/test_timeouts.cc (right):

http://codereview.chromium.org/8368009/diff/6017/base/test/test_timeouts.cc#n...
base/test/test_timeouts.cc:69: DCHECK(tiny_timeout_ms_ <= action_timeout_ms_);
Is this actually test code, where an ASSERT... would be better?

http://codereview.chromium.org/8368009/diff/6017/base/threading/non_thread_sa...
File base/threading/non_thread_safe_unittest.cc (right):

http://codereview.chromium.org/8368009/diff/6017/base/threading/non_thread_sa...
base/threading/non_thread_safe_unittest.cc:23: DCHECK(CalledOnValidThread());
Better would be ASSERT, since this is a test file.

http://codereview.chromium.org/8368009/diff/6017/base/threading/thread_local_...
File base/threading/thread_local_posix.cc (right):

http://codereview.chromium.org/8368009/diff/6017/base/threading/thread_local_...
base/threading/thread_local_posix.cc:18: DCHECK_EQ(error, 0);
There is a chance that we'll exhaust TLS slots, and that might only happen when
enough features are run at the same time (and not have popped up in unit tests).
Getting messed up (and not realizing we've exhausted this list) seems like a
very Bad Thing (TM), and hence worth a CHECK.

http://codereview.chromium.org/8368009/diff/6017/base/threading/thread_local_...
base/threading/thread_local_posix.cc:35: DCHECK_EQ(error, 0);
I'm fine with this, as it should only happen when hell has broken loose, or when
the logic is hosed (and we didn't ask for a slot first).  I believe we should
have caught such in unit tests... so this can be a DCHECK (I think).

http://codereview.chromium.org/8368009/diff/6017/base/threading/thread_local_...
File base/threading/thread_local_win.cc (right):

http://codereview.chromium.org/8368009/diff/6017/base/threading/thread_local_...
base/threading/thread_local_win.cc:18: DCHECK_NE(slot, TLS_OUT_OF_INDEXES);
Again... I think we should stay with CHECK here.

http://codereview.chromium.org/8368009/diff/6017/base/threading/thread_local_...
base/threading/thread_local_win.cc:36: DLOG(FATAL) << "Failed to
TlsSetValue().";
This is fine IMO (as mentioned in posix file).

http://codereview.chromium.org/8368009/diff/6017/base/tracked_objects.cc
File base/tracked_objects.cc (right):

http://codereview.chromium.org/8368009/diff/6017/base/tracked_objects.cc#newc...
base/tracked_objects.cc:390: DCHECK(tls_index_.Initialize(NULL));
The call to Initialize(...) is for side effects (it does the initialization), so
we need it one way or the other.

I'm editing the heck out of this file currently... and the above line is going
away... so I'd say just leave it as is (I promise the CHECK won't be there).

[brettw, 2011-10-23 02:32:58]

Thanks for the careful review. I addressed everything (I hope!).

[brettw, 2011-10-23 02:53:50]

http://codereview.chromium.org/8368009/diff/7019/base/debug/trace_event_unitt...
File base/debug/trace_event_unittest.cc (right):

http://codereview.chromium.org/8368009/diff/7019/base/debug/trace_event_unitt...
base/debug/trace_event_unittest.cc:104: ASSERT_TRUE(false);
I just ended up reverting this file. The compiler doesn't like
ASSERT_TRUE(false) and since this is a test, I don't care enough to mess with it
too much.

[jar (doing other things), 2011-10-23 15:58:00]

LGTM... but I could have missed something (a call with side effects).  Be sure
to watch the landing... as a revert may be needed.

Nice job!