Reorganizing few DCHECKs for better catching of failure.

content/browser/browser_thread_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/browser_thread_impl.h"
 
 #include <string.h>
 
 #include <string>
 
@@ skipping 140 matching lines @@
   // during shutdown get run. There's nothing particularly scientific about the
   // number chosen.
   const int kMaxNewShutdownBlockingTasks = 1000;
   BrowserThreadGlobals& globals = g_globals.Get();
   globals.blocking_pool->Shutdown(kMaxNewShutdownBlockingTasks);
 }
 
 // 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();
 }
 
 void BrowserThreadImpl::Init() {
   BrowserThreadGlobals& globals = g_globals.Get();
 
   using base::subtle::AtomicWord;
   AtomicWord* storage =
@@ skipping 107 matching lines @@
       reinterpret_cast<BrowserThreadDelegate*>(stored_pointer);
 
   if (delegate)
     delegate->CleanUp();
 
   // PostTaskHelper() accesses the message loop while holding this lock.
   // However, the message loop will soon be destructed without any locking. So
   // 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;
 }
 
 BrowserThreadImpl::~BrowserThreadImpl() {
   // All Thread subclasses must call Stop() in the destructor. This is
   // doubly important here as various bits of code check they are on
   // the right BrowserThread.
   Stop();
 
   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) {
     DCHECK(!globals.threads[i]) <<
         "Threads must be listed in the reverse order that they die";
   }
 #endif
 }
 
 bool BrowserThreadImpl::StartWithOptions(const Options& options) {
   // The global thread table needs to be locked while a new thread is
   // starting, as the new thread can asynchronously start touching the
   // table (and other thread's message_loop).
   BrowserThreadGlobals& globals = g_globals.Get();
   base::AutoLock lock(globals.lock);
   return Thread::StartWithOptions(options);
 }
 
 // static
 bool BrowserThreadImpl::PostTaskHelper(
     BrowserThread::ID identifier,
     const tracked_objects::Location& from_here,
     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.
   // Note: since the array is so small, ok to loop instead of creating a map,
   // which would require a lock because std::map isn't thread safe, defeating
   // the whole purpose of this optimization.
   BrowserThread::ID current_thread = ID_COUNT;
   bool target_thread_outlives_current =
       GetCurrentThreadIdentifier(&current_thread) &&
       current_thread >= identifier;
 
   BrowserThreadGlobals& globals = g_globals.Get();
   if (!target_thread_outlives_current)
     globals.lock.Acquire();
 
   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);
     } else {
       message_loop->task_runner()->PostNonNestableDelayedTask(from_here, task,
                                                               delay);
     }
   }
 
   if (!target_thread_outlives_current)
@@ skipping 36 matching lines @@
                                                       task);
 }
 
 // static
 base::SequencedWorkerPool* BrowserThread::GetBlockingPool() {
   return g_globals.Get().blocking_pool.get();
 }
 
 // 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();
 }
 
 // static
 std::string BrowserThread::GetDCheckCurrentlyOnErrorMessage(ID expected) {
   std::string actual_name = base::PlatformThread::GetName();
   if (actual_name.empty())
     actual_name = "Unknown Thread";
 
   std::string result = "Must be called on ";
   result += GetThreadName(expected);
   result += "; actually called on ";
   result += actual_name;
   result += ".";
   return result;
 }
 
 // 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();
 }
 
 // static
 bool BrowserThread::PostTask(ID identifier,
                              const tracked_objects::Location& from_here,
                              const base::Closure& task) {
   return BrowserThreadImpl::PostTaskHelper(
       identifier, from_here, task, base::TimeDelta(), true);
@@ skipping 32 matching lines @@
     ID identifier,
     const tracked_objects::Location& from_here,
     const base::Closure& task,
     const base::Closure& reply) {
   return GetTaskRunnerForThread(identifier)
       ->PostTaskAndReply(from_here, task, reply);
 }
 
 // static
 bool BrowserThread::GetCurrentThreadIdentifier(ID* identifier) {
-  if (g_globals == NULL)
+  if (g_globals == nullptr)
     return false;
 
   base::MessageLoop* cur_message_loop = base::MessageLoop::current();
   BrowserThreadGlobals& globals = g_globals.Get();
   // Profiler to track potential contention on |globals.lock|. This only does
   // real work on canary and local dev builds, so the cost of having this here
   // should be minimal.
   tracked_objects::ScopedTracker tracking_profile(FROM_HERE);
   base::AutoLock lock(globals.lock);
   for (int i = 0; i < ID_COUNT; ++i) {
     if (globals.threads[i] &&
         globals.threads[i]->message_loop() == cur_message_loop) {
       *identifier = globals.threads[i]->identifier_;
       return true;
     }
   }
 
   return false;
 }
 
 // static
 scoped_refptr<base::SingleThreadTaskRunner>
 BrowserThread::GetTaskRunnerForThread(ID identifier) {
   return g_task_runners.Get().proxies[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);
   base::Thread* thread = globals.threads[identifier];
   DCHECK(thread);
   base::MessageLoop* loop = thread->message_loop();
   return loop;
 }
 
 // static
 void BrowserThread::SetDelegate(ID identifier,
                                 BrowserThreadDelegate* delegate) {
   using base::subtle::AtomicWord;
   BrowserThreadGlobals& globals = g_globals.Get();
   AtomicWord* storage = reinterpret_cast<AtomicWord*>(
       &globals.thread_delegates[identifier]);
   AtomicWord old_pointer = base::subtle::NoBarrier_AtomicExchange(
       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