Reorganizing few DCHECKs for better catching of failure.

content/browser/browser_thread_impl.cc

Index: content/browser/browser_thread_impl.cc
diff --git a/content/browser/browser_thread_impl.cc b/content/browser/browser_thread_impl.cc
index 632b0b555663ee2d737a08d619486a69ae29dd76..d38a9f96ac99ae871db2f093a8fbed9607d0fd70 100644
--- a/content/browser/browser_thread_impl.cc
+++ b/content/browser/browser_thread_impl.cc
@@ -158,7 +158,7 @@ void BrowserThreadImpl::ShutdownThreadPool() {
 // static
 void BrowserThreadImpl::FlushThreadPoolHelperForTesting() {
   // We don't want to create a pool if none exists.
-  if (g_globals == NULL)
+  if (g_globals == nullptr)
     return;
   g_globals.Get().blocking_pool->FlushForTesting();
   disk_cache::SimpleBackendImpl::FlushWorkerPoolForTesting();
@@ -286,15 +286,16 @@ void BrowserThreadImpl::CleanUp() {
   // to prevent a race with accessing the message loop in PostTaskHelper(),
   // remove this thread from the global array now.
   base::AutoLock lock(globals.lock);
-  globals.threads[identifier_] = NULL;
+  globals.threads[identifier_] = nullptr;
 }
 
 void BrowserThreadImpl::Initialize() {
   BrowserThreadGlobals& globals = g_globals.Get();
 
   base::AutoLock lock(globals.lock);
-  DCHECK(identifier_ >= 0 && identifier_ < ID_COUNT);
-  DCHECK(globals.threads[identifier_] == NULL);
+  DCHECK(identifier_ >= 0);

[no sievers, 2016/07/21 19:08:01]

nit: you can even do DCHECK_GE(identifier, 0) which will additionally print the
value of |identifier_| if it fails.

Similarly for DCHECK_LT(identifier_, ID_COUNT);

[AKV, 2016/07/22 14:26:13]

Done. Thanks, that's very helpful in catching failures.

+  DCHECK(identifier_ < ID_COUNT);
+  DCHECK(globals.threads[identifier_] == nullptr);
   globals.threads[identifier_] = this;
 }
 
@@ -306,7 +307,7 @@ BrowserThreadImpl::~BrowserThreadImpl() {
 
   BrowserThreadGlobals& globals = g_globals.Get();
   base::AutoLock lock(globals.lock);
-  globals.threads[identifier_] = NULL;
+  globals.threads[identifier_] = nullptr;
 #ifndef NDEBUG
   // Double check that the threads are ordered correctly in the enumeration.
   for (int i = identifier_ + 1; i < ID_COUNT; ++i) {
@@ -332,7 +333,8 @@ bool BrowserThreadImpl::PostTaskHelper(
     const base::Closure& task,
     base::TimeDelta delay,
     bool nestable) {
-  DCHECK(identifier >= 0 && identifier < ID_COUNT);
+  DCHECK(identifier >= 0);
+  DCHECK(identifier < ID_COUNT);
   // Optimization: to avoid unnecessary locks, we listed the ID enumeration in
   // order of lifetime.  So no need to lock if we know that the target thread
   // outlives current thread.
@@ -350,7 +352,7 @@ bool BrowserThreadImpl::PostTaskHelper(
 
   base::MessageLoop* message_loop =
       globals.threads[identifier] ? globals.threads[identifier]->message_loop()
-                                  : NULL;
+                                  : nullptr;
   if (message_loop) {
     if (nestable) {
       message_loop->task_runner()->PostDelayedTask(from_here, task, delay);
@@ -407,20 +409,22 @@ base::SequencedWorkerPool* BrowserThread::GetBlockingPool() {
 
 // static
 bool BrowserThread::IsThreadInitialized(ID identifier) {
-  if (g_globals == NULL)
+  if (g_globals == nullptr)
     return false;
 
   BrowserThreadGlobals& globals = g_globals.Get();
   base::AutoLock lock(globals.lock);
-  DCHECK(identifier >= 0 && identifier < ID_COUNT);
-  return globals.threads[identifier] != NULL;
+  DCHECK(identifier >= 0);
+  DCHECK(identifier < ID_COUNT);
+  return globals.threads[identifier] != nullptr;
 }
 
 // static
 bool BrowserThread::CurrentlyOn(ID identifier) {
   BrowserThreadGlobals& globals = g_globals.Get();
   base::AutoLock lock(globals.lock);
-  DCHECK(identifier >= 0 && identifier < ID_COUNT);
+  DCHECK(identifier >= 0);
+  DCHECK(identifier < ID_COUNT);
   return globals.threads[identifier] &&
          globals.threads[identifier]->message_loop() ==
              base::MessageLoop::current();
@@ -442,12 +446,13 @@ std::string BrowserThread::GetDCheckCurrentlyOnErrorMessage(ID expected) {
 
 // static
 bool BrowserThread::IsMessageLoopValid(ID identifier) {
-  if (g_globals == NULL)
+  if (g_globals == nullptr)
     return false;
 
   BrowserThreadGlobals& globals = g_globals.Get();
   base::AutoLock lock(globals.lock);
-  DCHECK(identifier >= 0 && identifier < ID_COUNT);
+  DCHECK(identifier >= 0);
+  DCHECK(identifier < ID_COUNT);
   return globals.threads[identifier] &&
          globals.threads[identifier]->message_loop();
 }
@@ -500,7 +505,7 @@ bool BrowserThread::PostTaskAndReply(
 
 // static
 bool BrowserThread::GetCurrentThreadIdentifier(ID* identifier) {
-  if (g_globals == NULL)
+  if (g_globals == nullptr)
     return false;
 
   base::MessageLoop* cur_message_loop = base::MessageLoop::current();
@@ -529,8 +534,8 @@ BrowserThread::GetTaskRunnerForThread(ID identifier) {
 
 // static
 base::MessageLoop* BrowserThread::UnsafeGetMessageLoopForThread(ID identifier) {
-  if (g_globals == NULL)
-    return NULL;
+  if (g_globals == nullptr)
+    return nullptr;
 
   BrowserThreadGlobals& globals = g_globals.Get();
   base::AutoLock lock(globals.lock);
@@ -551,7 +556,8 @@ void BrowserThread::SetDelegate(ID identifier,
       storage, reinterpret_cast<AtomicWord>(delegate));
 
   // This catches registration when previously registered.
-  DCHECK(!delegate || !old_pointer);
+  DCHECK(!delegate);
+  DCHECK(!old_pointer);

[no sievers, 2016/07/21 19:08:01]

This is wrong. The current code basically checks that if you pass a non-null
delegate, you can't have an old_pointer (previously registered).

[AKV, 2016/07/22 14:26:13]

Done. Thanks.

 }
 
 }  // namespace content