Make CHECK_EQ and friends work properly in an if/else structure without braces.

base/logging.h

Index: base/logging.h
diff --git a/base/logging.h b/base/logging.h
index ea096d19f729cb9043b0a6fcaea7a935e0d943aa..bd1e7fb59502f99ee06e28dedcdfcc466bc2a567 100644
--- a/base/logging.h
+++ b/base/logging.h
@@ -480,16 +480,37 @@ const LogSeverity LOG_0 = LOG_ERROR;
 
 #endif  // _PREFAST_
 
+// Captures the result of a CHECK_EQ (for example) and facilitates testing as a
+// boolean.
+class CheckOpResult {
+ public:
+  // |message| must be null if and only if the check failed.
+  CheckOpResult(std::string* message) : message_(message) {}
+  // Returns true if the check succeeded.
+  operator bool() const { return !message_; }
+  // Returns the message.
+  std::string* message() { return message_; }
+
+ private:
+  std::string* message_;
+};

[Nico, 2015/07/06 22:41:52]

why is this needed?

[erikwright (departed), 2015/07/07 17:25:08]

It allows us to both store the result of logging::Check##name##Impl((val1),
(val2), ...) and also compare it to null.

If the check fails, the result is a string that incorporates the results of
evaluating val1 and val2. Otherwise it is null.

I tried 'if ((std::string* _result = ...) == nullptr) ... else ...' but it does
not work.

+
 // Helper macro for binary operators.
 // Don't use this macro directly in your code, use CHECK_EQ et al below.
-//
-// TODO(akalin): Rewrite this so that constructs like if (...)
-// CHECK_EQ(...) else { ... } work properly.
-#define CHECK_OP(name, op, val1, val2)                          \
-  if (std::string* _result =                                    \
-      logging::Check##name##Impl((val1), (val2),                \
-                                 #val1 " " #op " " #val2))      \
-    logging::LogMessage(__FILE__, __LINE__, _result).stream()
+// The 'switch' is used to prevent the 'if' from being ambiguous when the macro
+// is used in an 'else' clause such as:
+// if (a == 1)
+//   statement;
+// else
+//   DCHECK_EQ(2, a);

[danakj, 2015/07/06 22:38:26]

But else if works fine, what is ambiguous?

[Nico, 2015/07/07 16:59:43]

I think the example is wrong and it's supposed to be

  if (a == 1)
    CHECK_EQ(2, a) << "foo";
  else
    statement;

[erikwright (departed), 2015/07/07 17:25:08]

You're correct, the example is wrong. I'm updating the comment and adding a test
case. The compiler error is:

../../base/logging_unittest.cc:424:5: error: add explicit braces to avoid
dangling else [-Werror,-Wdangling-else]
    DCHECK_EQ(false, reached = true);
    ^
../../base/logging.h:723:31: note: expanded from macro 'DCHECK_EQ'
#define DCHECK_EQ(val1, val2) DCHECK_OP(EQ, ==, val1, val2)
                              ^
../../base/logging.h:700:3: note: expanded from macro 'DCHECK_OP'
  else                                                                \
  ^
1 error generated.

+#define CHECK_OP(name, op, val1, val2)                                   \

[Nico, 2015/07/06 22:41:52]

isn't the usual pattern for this

  do { /* stuff */ } while (0)

?

[danakj, 2015/07/06 22:43:27]

Ya, that's what I've seen elsewhere too.

[erikwright (departed), 2015/07/07 17:25:08]

That doesn't work because you need the possible '<< "message"' to be part of
'stuff'.

+  switch (0) case 0: default:                                            \
+  if (logging::CheckOpResult _result =                                   \
+      logging::Check##name##Impl((val1), (val2),                         \
+                                 #val1 " " #op " " #val2))               \
+   ;                                                                     \
+  else                                                                   \
+    logging::LogMessage(__FILE__, __LINE__, _result.message()).stream()
 
 #endif
 
@@ -665,12 +686,22 @@ const LogSeverity LOG_DCHECK = LOG_INFO;
 
 // 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 (DCHECK_IS_ON())                                                     \
-    if (std::string* _result = logging::Check##name##Impl(                \
-            (val1), (val2), #val1 " " #op " " #val2))                     \
-  logging::LogMessage(__FILE__, __LINE__, ::logging::LOG_DCHECK, _result) \
-      .stream()
+// The 'switch' is used to prevent the 'if' from being ambiguous when the macro
+// is used in an 'else' clause such as:
+// if (a == 1)
+//   statement;
+// else
+//   DCHECK_EQ(2, a);
+#define DCHECK_OP(name, op, val1, val2)                               \
+  switch (0) case 0: default:                                         \
+  if (logging::CheckOpResult _result =                                \
+      DCHECK_IS_ON() ?                                                \
+      logging::Check##name##Impl((val1), (val2),                      \
+                                 #val1 " " #op " " #val2) : nullptr)  \
+   ;                                                                  \
+  else                                                                \
+    logging::LogMessage(__FILE__, __LINE__, ::logging::LOG_DCHECK,    \
+                        _result.message()).stream()
 
 // Equality/Inequality checks - compare two values, and log a
 // LOG_DCHECK message including the two values when the result is not