Fixed subtle difference in behavior between DCHECK and DCHECK_EQ et al.

BUG=58998
TEST=New unit tests

Review URL: http://codereview.chromium.org/4199005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@64650 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 59 insertions, 14 deletions

diff --git a/base/logging.h b/base/logging.h
index 6704325..4b3caca 100644
--- a/base/logging.h
+++ b/base/logging.h
@@ -531,6 +531,7 @@ DECLARE_CHECK_STROP_IMPL(_stricmp, false)
 
 #if ENABLE_DLOG
 
+#define DLOG_IS_ON(severity) LOG_IS_ON(severity)
 #define DLOG_IF(severity, condition) LOG_IF(severity, condition)
 #define DLOG_ASSERT(condition) LOG_ASSERT(condition)
 #define DPLOG_IF(severity, condition) PLOG_IF(severity, condition)
@@ -546,6 +547,7 @@ DECLARE_CHECK_STROP_IMPL(_stricmp, false)
 #define DLOG_EAT_STREAM_PARAMETERS                                      \
   true ? (void) 0 : ::logging::LogMessageVoidify() & LOG_STREAM(FATAL)
 
+#define DLOG_IS_ON(severity) false
 #define DLOG_IF(severity, condition) DLOG_EAT_STREAM_PARAMETERS
 #define DLOG_ASSERT(condition) DLOG_EAT_STREAM_PARAMETERS
 #define DPLOG_IF(severity, condition) DLOG_EAT_STREAM_PARAMETERS
@@ -565,8 +567,6 @@ enum { DEBUG_MODE = ENABLE_DLOG };
 
 #undef ENABLE_DLOG
 
-#define DLOG_IS_ON(severity) (::logging::DEBUG_MODE && LOG_IS_ON(severity))
-
 #define DLOG(severity)                                          \
   LAZY_STREAM(LOG_STREAM(severity), DLOG_IS_ON(severity))
 
@@ -592,28 +592,34 @@ enum { DEBUG_MODE = ENABLE_DLOG };
 
 #if defined(NDEBUG)
 
-// Set to true in InitLogging when we want to enable the dchecks in release.
-extern bool g_enable_dcheck;
-#define DCHECK_IS_ON() (::logging::g_enable_dcheck)
 #define DCHECK_SEVERITY ERROR_REPORT
 const LogSeverity LOG_DCHECK = LOG_ERROR_REPORT;
+// This is set to true in InitLogging when we want to enable the
+// DCHECKs in release.
+extern bool g_enable_dcheck;
+#define DCHECK_IS_ON() (::logging::g_enable_dcheck && LOG_IS_ON(DCHECK))
 
 #else  // defined(NDEBUG)
 
-// On a regular debug build, we want to have DCHECKS enabled.
-#define DCHECK_IS_ON() (true)
+// On a regular debug build, we want to have DCHECKs enabled.
 #define DCHECK_SEVERITY FATAL
 const LogSeverity LOG_DCHECK = LOG_FATAL;
+// TODO(akalin): We don't define this as 'true' since if the log level
+// is above FATAL, the DCHECK won't go through anyway.  Make it so
+// that DCHECKs work regardless of the logging level, then set this to
+// 'true'.
+#define DCHECK_IS_ON() LOG_IS_ON(DCHECK)
 
 #endif  // defined(NDEBUG)
 
 #else  // ENABLE_DCHECK
 
-#define DCHECK_IS_ON() (false)
 #define DCHECK_SEVERITY FATAL
 const LogSeverity LOG_DCHECK = LOG_FATAL;
+#define DCHECK_IS_ON() false
 
 #endif  // ENABLE_DCHECK
+#undef ENABLE_DCHECK
 
 // Unlike CHECK et al., DCHECK et al. *does* evaluate their arguments
 // lazily.
@@ -636,7 +642,7 @@ const LogSeverity LOG_DCHECK = LOG_FATAL;
 // Helper macro for binary operators.
 // Don't use this macro directly in your code, use DCHECK_EQ et al below.
 #define DCHECK_OP(name, op, val1, val2)                         \
-  if (DLOG_IS_ON(DCHECK_SEVERITY))                              \
+  if (DCHECK_IS_ON())                                           \
     if (logging::CheckOpString _result =                        \
         logging::Check##name##Impl((val1), (val2),              \
                                    #val1 " " #op " " #val2))    \
diff --git a/base/logging_unittest.cc b/base/logging_unittest.cc
index fa28ec6..e03c45e 100644
--- a/base/logging_unittest.cc
+++ b/base/logging_unittest.cc
@@ -14,23 +14,35 @@ namespace {
 
 using ::testing::Return;
 
+// Needs to be global since log assert handlers can't maintain state.
+int log_sink_call_count = 0;
+
+void LogSink(const std::string& str) {
+  ++log_sink_call_count;
+}
+
 // Class to make sure any manipulations we do to the min log level are
 // contained (i.e., do not affect other unit tests).
-class MinLogLevelSaver {
+class LogStateSaver {
  public:
-  MinLogLevelSaver() : old_min_log_level_(GetMinLogLevel()) {}
+  LogStateSaver() : old_min_log_level_(GetMinLogLevel()) {}
 
-  ~MinLogLevelSaver() { SetMinLogLevel(old_min_log_level_);  }
+  ~LogStateSaver() {
+    SetMinLogLevel(old_min_log_level_);
+    SetLogAssertHandler(NULL);
+    SetLogReportHandler(NULL);
+    log_sink_call_count = 0;
+  }
 
  private:
   int old_min_log_level_;
 
-  DISALLOW_COPY_AND_ASSIGN(MinLogLevelSaver);
+  DISALLOW_COPY_AND_ASSIGN(LogStateSaver);
 };
 
 class LoggingTest : public testing::Test {
  private:
-  MinLogLevelSaver min_log_level_saver_;
+  LogStateSaver log_state_saver_;
 };
 
 class MockLogSource {
@@ -140,6 +152,33 @@ TEST_F(LoggingTest, DchecksAreLazy) {
       << mock_log_source.Log();
 }
 
+TEST_F(LoggingTest, Dcheck) {
+#if defined(LOGGING_IS_OFFICIAL_BUILD)
+  // Official build.
+  EXPECT_FALSE(DCHECK_IS_ON());
+  EXPECT_FALSE(DLOG_IS_ON(DCHECK));
+#elif defined(NDEBUG)
+  // Unofficial release build.
+  logging::g_enable_dcheck = true;
+  logging::SetLogReportHandler(&LogSink);
+  EXPECT_TRUE(DCHECK_IS_ON());
+  EXPECT_FALSE(DLOG_IS_ON(DCHECK));
+#else
+  // Unofficial debug build.
+  logging::SetLogAssertHandler(&LogSink);
+  EXPECT_TRUE(DCHECK_IS_ON());
+  EXPECT_TRUE(DLOG_IS_ON(DCHECK));
+#endif  // defined(LOGGING_IS_OFFICIAL_BUILD)
+
+  EXPECT_EQ(0, log_sink_call_count);
+  DCHECK(false);
+  EXPECT_EQ(DCHECK_IS_ON() ? 1 : 0, log_sink_call_count);
+  DPCHECK(false);
+  EXPECT_EQ(DCHECK_IS_ON() ? 2 : 0, log_sink_call_count);
+  DCHECK_EQ(0, 1);
+  EXPECT_EQ(DCHECK_IS_ON() ? 3 : 0, log_sink_call_count);
+}
+
 TEST_F(LoggingTest, DcheckReleaseBehavior) {
   int some_variable = 1;
   // These should still reference |some_variable| so we don't get