Move RTC_CHECK_OP error message construction out of header file.

This simplifies the logic, prevents emitting code for every pair of
argument types to RTC_CHECK_OP and partially unblocks removing streams from
the check code altogether.

Bug: webrtc:8982
Change-Id: Ib6652ac9a342e4471c12574a79872833cc943407
Reviewed-on: https://webrtc-review.googlesource.com/86544
Commit-Queue: Jonas Olsson <jonasolsson@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23821}

rtc_base/checks.cc

@@ -36,22 +36,58 @@
 #include "rtc_base/checks.h"
 
 namespace rtc {
-
-// MSVC doesn't like complex extern templates and DLLs.
-#if !defined(COMPILER_MSVC)
-// Explicit instantiations for commonly used comparisons.
-template std::string* MakeCheckOpString<int, int>(
-    const int&, const int&, const char* names);
-template std::string* MakeCheckOpString<unsigned long, unsigned long>(
-    const unsigned long&, const unsigned long&, const char* names);
-template std::string* MakeCheckOpString<unsigned long, unsigned int>(
-    const unsigned long&, const unsigned int&, const char* names);
-template std::string* MakeCheckOpString<unsigned int, unsigned long>(
-    const unsigned int&, const unsigned long&, const char* names);
-template std::string* MakeCheckOpString<std::string, std::string>(
-    const std::string&, const std::string&, const char* name);
-#endif
 namespace webrtc_checks_impl {
+
+// Reads one argument from args, appends it to s and advances fmt.
+// Returns true iff an argument was sucessfully parsed.
+bool ParseArg(va_list* args,
+              const CheckArgType** fmt,
+              std::ostream& s) {  // no-presubmit-check TODO(webrtc:8982)
+  if (**fmt == CheckArgType::kEnd)
+    return false;
+
+  switch (**fmt) {
+    case CheckArgType::kInt:
+      s << va_arg(*args, int);
+      break;
+    case CheckArgType::kLong:
+      s << va_arg(*args, long);
+      break;
+    case CheckArgType::kLongLong:
+      s << va_arg(*args, long long);
+      break;
+    case CheckArgType::kUInt:
+      s << va_arg(*args, unsigned);
+      break;
+    case CheckArgType::kULong:
+      s << va_arg(*args, unsigned long);
+      break;
+    case CheckArgType::kULongLong:
+      s << va_arg(*args, unsigned long long);
+      break;
+    case CheckArgType::kDouble:
+      s << va_arg(*args, double);
+      break;
+    case CheckArgType::kLongDouble:
+      s << va_arg(*args, long double);
+      break;
+    case CheckArgType::kCharP:
+      s << va_arg(*args, const char*);
+      break;
+    case CheckArgType::kStdString:
+      s << *va_arg(*args, const std::string*);
+      break;
+    case CheckArgType::kVoidP:
+      s << reinterpret_cast<std::uintptr_t>(va_arg(*args, const void*));
+      break;
+    default:
+      s << "[Invalid CheckArgType:" << static_cast<int8_t>(**fmt) << "]";
+      return false;
+  }
+  (*fmt)++;
+  return true;
+}
+
 RTC_NORETURN void FatalLog(const char* file,
                            int line,
                            const char* message,
@@ -62,50 +98,25 @@ RTC_NORETURN void FatalLog(const char* file,
 
   std::ostringstream ss;  // no-presubmit-check TODO(webrtc:8982)
   ss << "\n\n#\n# Fatal error in: " << file << ", line " << line
-     << "\n# last system error: " << LAST_SYSTEM_ERROR
+     << "\n# last system error: " << LAST_SYSTEM_ERROR << "\n# Check failed: ";
-     << "\n# Check failed: " << message << "\n# ";
 
-  for (; *fmt != CheckArgType::kEnd; ++fmt) {
+  if (*fmt == CheckArgType::kCheckOp) {
-    switch (*fmt) {
+    // This log message was generated by RTC_CHECK_OP, so we have to complete
-      case CheckArgType::kInt:
+    // the error message using the operands that have been passed as the first
-        ss << va_arg(args, int);
+    // two arguments.
-        break;
+    fmt++;
-      case CheckArgType::kLong:
+
-        ss << va_arg(args, long);
+    std::ostringstream s1, s2;  // no-presubmit-check TODO(webrtc:8982)
-        break;
+    if (ParseArg(&args, &fmt, s1) && ParseArg(&args, &fmt, s2))
-      case CheckArgType::kLongLong:
+      ss << message << " (" << s1.str() << " vs. " << s2.str() << ")\n# ";
-        ss << va_arg(args, long long);
+  } else {
-        break;
+    ss << message << "\n# ";
-      case CheckArgType::kUInt:
-        ss << va_arg(args, unsigned);
-        break;
-      case CheckArgType::kULong:
-        ss << va_arg(args, unsigned long);
-        break;
-      case CheckArgType::kULongLong:
-        ss << va_arg(args, unsigned long long);
-        break;
-      case CheckArgType::kDouble:
-        ss << va_arg(args, double);
-        break;
-      case CheckArgType::kLongDouble:
-        ss << va_arg(args, long double);
-        break;
-      case CheckArgType::kCharP:
-        ss << va_arg(args, const char*);
-        break;
-      case CheckArgType::kStdString:
-        ss << *va_arg(args, const std::string*);
-        break;
-      case CheckArgType::kVoidP:
-        ss << va_arg(args, const void*);
-        break;
-      default:
-        ss << "[Invalid CheckArgType:" << static_cast<int8_t>(*fmt) << "]";
-        goto processing_loop_end;
-    }
   }
-processing_loop_end:
+
+  // Append all the user-supplied arguments to the message.
+  while (ParseArg(&args, &fmt, ss))
+    ;
+
   va_end(args);
 
   std::string s = ss.str();

rtc_base/checks.h

@@ -40,7 +40,7 @@ RTC_NORETURN void rtc_FatalMessage(const char* file, int line, const char* msg);
 #ifdef __cplusplus
 // C++ version.
 
-#include <sstream>
+#include <sstream>  // no-presubmit-check TODO(webrtc:8982)
 #include <string>
 
 #include "rtc_base/numerics/safe_compare.h"
@@ -101,6 +101,12 @@ enum class CheckArgType : int8_t {
   kCharP,
   kStdString,
   kVoidP,
+
+  // kCheckOp doesn't represent an argument type. Instead, it is sent as the
+  // first argument from RTC_CHECK_OP to make FatalLog use the next two
+  // arguments to build the special CHECK_OP error message
+  // (the "a == b (1 vs. 2)" bit).
+  kCheckOp,
 };
 
 RTC_NORETURN void FatalLog(const char* file,
@@ -191,6 +197,16 @@ class LogStreamer<> final {
     static constexpr CheckArgType t[] = {Us::Type()..., CheckArgType::kEnd};
     FatalLog(file, line, message, t, args.GetVal()...);
   }
+
+  template <typename... Us>
+  RTC_NORETURN RTC_FORCE_INLINE static void CallCheckOp(const char* file,
+                                                        const int line,
+                                                        const char* message,
+                                                        const Us&... args) {
+    static constexpr CheckArgType t[] = {CheckArgType::kCheckOp, Us::Type()...,
+                                         CheckArgType::kEnd};
+    FatalLog(file, line, message, t, args.GetVal()...);
+  }
 };
 
 // Inductive case: We've already seen at least one << argument. The most recent
@@ -227,6 +243,14 @@ class LogStreamer<T, Ts...> final {
     prior_->Call(file, line, message, arg_, args...);
   }
 
+  template <typename... Us>
+  RTC_NORETURN RTC_FORCE_INLINE void CallCheckOp(const char* file,
+                                                 const int line,
+                                                 const char* message,
+                                                 const Us&... args) const {
+    prior_->CallCheckOp(file, line, message, arg_, args...);
+  }
+
  private:
   // The most recent argument.
   T arg_;
@@ -235,6 +259,7 @@ class LogStreamer<T, Ts...> final {
   const LogStreamer<Ts...>* prior_;
 };
 
+template <bool isCheckOp>
 class FatalLogCall final {
  public:
   FatalLogCall(const char* file, int line, const char* message)
@@ -244,7 +269,8 @@ class FatalLogCall final {
   template <typename... Ts>
   RTC_NORETURN RTC_FORCE_INLINE void operator&(
       const LogStreamer<Ts...>& streamer) {
-    streamer.Call(file_, line_, message_);
+    isCheckOp ? streamer.CallCheckOp(file_, line_, message_)
+              : streamer.Call(file_, line_, message_);
   }
 
  private:
@@ -259,10 +285,10 @@ class FatalLogCall final {
 // in a particularly convoluted way with an extra ?: because that appears to be
 // the simplest construct that keeps Visual Studio from complaining about
 // condition being unused).
-#define RTC_EAT_STREAM_PARAMETERS(ignored)                 \
+#define RTC_EAT_STREAM_PARAMETERS(ignored)                        \
-  (true ? true : ((void)(ignored), true))                  \
+  (true ? true : ((void)(ignored), true))                         \
-      ? static_cast<void>(0)                               \
+      ? static_cast<void>(0)                                      \
-      : rtc::webrtc_checks_impl::FatalLogCall("", 0, "") & \
+      : rtc::webrtc_checks_impl::FatalLogCall<false>("", 0, "") & \
             rtc::webrtc_checks_impl::LogStreamer<>()
 
 // Call RTC_EAT_STREAM_PARAMETERS with an argument that fails to compile if
@@ -277,80 +303,19 @@ class FatalLogCall final {
 //
 // We make sure RTC_CHECK et al. always evaluates |condition|, as
 // doing RTC_CHECK(FunctionWithSideEffect()) is a common idiom.
-#define RTC_CHECK(condition)                                              \
+#define RTC_CHECK(condition)                                       \
-  while (!(condition))                                                    \
+  while (!(condition))                                             \
-  rtc::webrtc_checks_impl::FatalLogCall(__FILE__, __LINE__, #condition) & \
+  rtc::webrtc_checks_impl::FatalLogCall<false>(__FILE__, __LINE__, \
+                                               #condition) &       \
       rtc::webrtc_checks_impl::LogStreamer<>()
 
 // Helper macro for binary operators.
 // Don't use this macro directly in your code, use RTC_CHECK_EQ et al below.
-// RTC_CHECK_EQ(...) else { ... } work properly.
+#define RTC_CHECK_OP(name, op, val1, val2)                               \
-#define RTC_CHECK_OP(name, op, val1, val2)                                    \
+  while (!rtc::Safe##name((val1), (val2)))                               \
-  while (std::string* _result =                                               \
+  rtc::webrtc_checks_impl::FatalLogCall<true>(__FILE__, __LINE__,        \
-             rtc::Check##name##Impl((val1), (val2), #val1 " " #op " " #val2)) \
+                                              #val1 " " #op " " #val2) & \
-  rtc::webrtc_checks_impl::FatalLogCall(__FILE__, __LINE__,                   \
+      rtc::webrtc_checks_impl::LogStreamer<>() << (val1) << (val2)
-                                        _result->c_str()) &                   \
-      rtc::webrtc_checks_impl::LogStreamer<>()
-
-// Build the error message string.  This is separate from the "Impl"
-// function template because it is not performance critical and so can
-// be out of line, while the "Impl" code should be inline.  Caller
-// takes ownership of the returned string.
-template<class t1, class t2>
-std::string* MakeCheckOpString(const t1& v1, const t2& v2, const char* names) {
-  // TODO(jonasolsson): Use absl::StrCat() instead.
-  std::ostringstream ss;
-  ss << names << " (" << v1 << " vs. " << v2 << ")";
-  std::string* msg = new std::string(ss.str());
-  return msg;
-}
-
-// MSVC doesn't like complex extern templates and DLLs.
-#if !defined(COMPILER_MSVC)
-// Commonly used instantiations of MakeCheckOpString<>. Explicitly instantiated
-// in logging.cc.
-extern template std::string* MakeCheckOpString<int, int>(
-    const int&, const int&, const char* names);
-extern template
-std::string* MakeCheckOpString<unsigned long, unsigned long>(
-    const unsigned long&, const unsigned long&, const char* names);
-extern template
-std::string* MakeCheckOpString<unsigned long, unsigned int>(
-    const unsigned long&, const unsigned int&, const char* names);
-extern template
-std::string* MakeCheckOpString<unsigned int, unsigned long>(
-    const unsigned int&, const unsigned long&, const char* names);
-extern template
-std::string* MakeCheckOpString<std::string, std::string>(
-    const std::string&, const std::string&, const char* name);
-#endif
-
-// Helper functions for RTC_CHECK_OP macro.
-// The (int, int) specialization works around the issue that the compiler
-// will not instantiate the template version of the function on values of
-// unnamed enum type - see comment below.
-#define DEFINE_RTC_CHECK_OP_IMPL(name)                                       \
-  template <class t1, class t2>                                              \
-  inline std::string* Check##name##Impl(const t1& v1, const t2& v2,          \
-                                        const char* names) {                 \
-    if (rtc::Safe##name(v1, v2))                                             \
-      return nullptr;                                                        \
-    else                                                                     \
-      return rtc::MakeCheckOpString(v1, v2, names);                          \
-  }                                                                          \
-  inline std::string* Check##name##Impl(int v1, int v2, const char* names) { \
-    if (rtc::Safe##name(v1, v2))                                             \
-      return nullptr;                                                        \
-    else                                                                     \
-      return rtc::MakeCheckOpString(v1, v2, names);                          \
-  }
-DEFINE_RTC_CHECK_OP_IMPL(Eq)
-DEFINE_RTC_CHECK_OP_IMPL(Ne)
-DEFINE_RTC_CHECK_OP_IMPL(Le)
-DEFINE_RTC_CHECK_OP_IMPL(Lt)
-DEFINE_RTC_CHECK_OP_IMPL(Ge)
-DEFINE_RTC_CHECK_OP_IMPL(Gt)
-#undef DEFINE_RTC_CHECK_OP_IMPL
 
 #define RTC_CHECK_EQ(val1, val2) RTC_CHECK_OP(Eq, ==, val1, val2)
 #define RTC_CHECK_NE(val1, val2) RTC_CHECK_OP(Ne, !=, val1, val2)
@@ -384,8 +349,9 @@ DEFINE_RTC_CHECK_OP_IMPL(Gt)
 #define RTC_NOTREACHED() RTC_DCHECK(RTC_UNREACHABLE_CODE_HIT)
 
 // TODO(bugs.webrtc.org/8454): Add an RTC_ prefix or rename differently.
-#define FATAL()                                                          \
+#define FATAL()                                                    \
-  rtc::webrtc_checks_impl::FatalLogCall(__FILE__, __LINE__, "FATAL()") & \
+  rtc::webrtc_checks_impl::FatalLogCall<false>(__FILE__, __LINE__, \
+                                               "FATAL()") &        \
       rtc::webrtc_checks_impl::LogStreamer<>()
 
 // Performs the integer division a/b and returns the result. CHECKs that the

rtc_base/logging_unittest.cc

@@ -194,38 +194,34 @@ TEST(LogTest, SingleStream) {
   EXPECT_EQ(sev, LogMessage::GetLogToStream(nullptr));
 }
 
-/*
 #if GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
 TEST(LogTest, Checks) {
   EXPECT_DEATH(FATAL() << "message",
                "\n\n#\n"
-               "# Fatal error in: \\S+, line \\d+\n"
+               "# Fatal error in: \\S+, line \\w+\n"
-               "# last system error: \\d+\n"
+               "# last system error: \\w+\n"
                "# Check failed: FATAL\\(\\)\n"
-               "# message"
+               "# message");
-               );
 
   int a = 1, b = 2;
   EXPECT_DEATH(RTC_CHECK_EQ(a, b) << 1 << 2u,
                "\n\n#\n"
-               "# Fatal error in: \\S+, line \\d+\n"
+               "# Fatal error in: \\S+, line \\w+\n"
-               "# last system error: \\d+\n"
+               "# last system error: \\w+\n"
                "# Check failed: a == b \\(1 vs. 2\\)\n"
-               "# 12"
+               "# 12");
-               );
   RTC_CHECK_EQ(5, 5);
 
   RTC_CHECK(true) << "Shouldn't crash" << 1;
   EXPECT_DEATH(RTC_CHECK(false) << "Hi there!",
                "\n\n#\n"
-               "# Fatal error in: \\S+, line \\d+\n"
+               "# Fatal error in: \\S+, line \\w+\n"
-               "# last system error: \\d+\n"
+               "# last system error: \\w+\n"
                "# Check failed: false\n"
-               "# Hi there!"
+               "# Hi there!");
-               );
 }
 #endif
-*/
+
 // Test using multiple log streams. The INFO stream should get the INFO message,
 // the VERBOSE stream should get the INFO and the VERBOSE.
 // We should restore the correct global state at the end.