Improve the implementation of CHECK() macro.
CHECK_EQ() currently does not work with smart pointers and nullptr.
This is an attempt to improve the situation by improving the
EagerEvaluator template. As a side benefit, the new implementation
no longer uses macros to generate template specializations.
The primary motivation of this change is to improve compatibility
with logging macros from google3 (Google's internal monorepo) and
make it easier to export code that was developed there to Android.
Bug: 151119760
Test: atest --host libbase_test
Change-Id: I9c957d5ddf2afb87a88304b1e4d7ceebea0fa2f8
diff --git a/include/android-base/logging.h b/include/android-base/logging.h
index 26827fb..ce0374b 100644
--- a/include/android-base/logging.h
+++ b/include/android-base/logging.h
@@ -251,13 +251,16 @@
// Helper for CHECK_xx(x,y) macros.
#define CHECK_OP(LHS, RHS, OP) \
for (auto _values = ::android::base::MakeEagerEvaluator(LHS, RHS); \
- UNLIKELY(!(_values.lhs OP _values.rhs)); \
+ UNLIKELY(!(_values.lhs.v OP _values.rhs.v)); \
/* empty */) \
ABORT_AFTER_LOG_FATAL \
::android::base::LogMessage(__FILE__, __LINE__, ::android::base::FATAL, _LOG_TAG_INTERNAL, -1) \
.stream() \
- << "Check failed: " << #LHS << " " << #OP << " " << #RHS << " (" #LHS "=" << _values.lhs \
- << ", " #RHS "=" << _values.rhs << ") "
+ << "Check failed: " << #LHS << " " << #OP << " " << #RHS << " (" #LHS "=" \
+ << ::android::base::LogNullGuard<decltype(_values.lhs.v)>::Guard(_values.lhs.v) \
+ << ", " #RHS "=" \
+ << ::android::base::LogNullGuard<decltype(_values.rhs.v)>::Guard(_values.rhs.v) \
+ << ") "
// clang-format on
// Check whether a condition holds between x and y, LOG(FATAL) if not. The value
@@ -347,49 +350,77 @@
#define DCHECK_CONSTEXPR(x, out, dummy) CHECK_CONSTEXPR(x, out, dummy)
#endif
+namespace log_detail {
+
+// Temporary storage for a single eagerly evaluated check expression operand.
+template <typename T> struct Storage {
+ template <typename U> explicit constexpr Storage(U&& u) : v(std::forward<U>(u)) {}
+ explicit Storage(const Storage& t) = delete;
+ explicit Storage(Storage&& t) = delete;
+ T v;
+};
+
+// Partial specialization for smart pointers to avoid copying.
+template <typename T> struct Storage<std::unique_ptr<T>> {
+ explicit constexpr Storage(const std::unique_ptr<T>& ptr) : v(ptr.get()) {}
+ const T* v;
+};
+template <typename T> struct Storage<std::shared_ptr<T>> {
+ explicit constexpr Storage(const std::shared_ptr<T>& ptr) : v(ptr.get()) {}
+ const T* v;
+};
+
+// Type trait that checks if a type is a (potentially const) char pointer.
+template <typename T> struct IsCharPointer {
+ using Pointee = std::remove_cv_t<std::remove_pointer_t<T>>;
+ static constexpr bool value = std::is_pointer_v<T> &&
+ (std::is_same_v<Pointee, char> || std::is_same_v<Pointee, signed char> ||
+ std::is_same_v<Pointee, unsigned char>);
+};
+
+// Counterpart to Storage that depends on both operands. This is used to prevent
+// char pointers being treated as strings in the log output - they might point
+// to buffers of unprintable binary data.
+template <typename LHS, typename RHS> struct StorageTypes {
+ static constexpr bool voidptr = IsCharPointer<LHS>::value && IsCharPointer<RHS>::value;
+ using LHSType = std::conditional_t<voidptr, const void*, LHS>;
+ using RHSType = std::conditional_t<voidptr, const void*, RHS>;
+};
+
// Temporary class created to evaluate the LHS and RHS, used with
// MakeEagerEvaluator to infer the types of LHS and RHS.
template <typename LHS, typename RHS>
struct EagerEvaluator {
- constexpr EagerEvaluator(LHS l, RHS r) : lhs(l), rhs(r) {
- }
- LHS lhs;
- RHS rhs;
+ template <typename A, typename B> constexpr EagerEvaluator(A&& l, B&& r)
+ : lhs(std::forward<A>(l)), rhs(std::forward<B>(r)) {}
+ const Storage<typename StorageTypes<LHS, RHS>::LHSType> lhs;
+ const Storage<typename StorageTypes<LHS, RHS>::RHSType> rhs;
+};
+
+} // namespace log_detail
+
+// Converts std::nullptr_t and null char pointers to the string "null"
+// when writing the failure message.
+template <typename T> struct LogNullGuard {
+ static const T& Guard(const T& v) { return v; }
+};
+template <> struct LogNullGuard<std::nullptr_t> {
+ static const char* Guard(const std::nullptr_t&) { return "(null)"; }
+};
+template <> struct LogNullGuard<char*> {
+ static const char* Guard(const char* v) { return v ? v : "(null)"; }
+};
+template <> struct LogNullGuard<const char*> {
+ static const char* Guard(const char* v) { return v ? v : "(null)"; }
};
// Helper function for CHECK_xx.
template <typename LHS, typename RHS>
-constexpr EagerEvaluator<LHS, RHS> MakeEagerEvaluator(LHS lhs, RHS rhs) {
- return EagerEvaluator<LHS, RHS>(lhs, rhs);
+constexpr auto MakeEagerEvaluator(LHS&& lhs, RHS&& rhs) {
+ return log_detail::EagerEvaluator<std::decay_t<LHS>, std::decay_t<RHS>>(
+ std::forward<LHS>(lhs), std::forward<RHS>(rhs));
}
-// Explicitly instantiate EagerEvalue for pointers so that char*s aren't treated
-// as strings. To compare strings use CHECK_STREQ and CHECK_STRNE. We rely on
-// signed/unsigned warnings to protect you against combinations not explicitly
-// listed below.
-#define EAGER_PTR_EVALUATOR(T1, T2) \
- template <> \
- struct EagerEvaluator<T1, T2> { \
- EagerEvaluator(T1 l, T2 r) \
- : lhs(reinterpret_cast<const void*>(l)), \
- rhs(reinterpret_cast<const void*>(r)) { \
- } \
- const void* lhs; \
- const void* rhs; \
- }
-EAGER_PTR_EVALUATOR(const char*, const char*);
-EAGER_PTR_EVALUATOR(const char*, char*);
-EAGER_PTR_EVALUATOR(char*, const char*);
-EAGER_PTR_EVALUATOR(char*, char*);
-EAGER_PTR_EVALUATOR(const unsigned char*, const unsigned char*);
-EAGER_PTR_EVALUATOR(const unsigned char*, unsigned char*);
-EAGER_PTR_EVALUATOR(unsigned char*, const unsigned char*);
-EAGER_PTR_EVALUATOR(unsigned char*, unsigned char*);
-EAGER_PTR_EVALUATOR(const signed char*, const signed char*);
-EAGER_PTR_EVALUATOR(const signed char*, signed char*);
-EAGER_PTR_EVALUATOR(signed char*, const signed char*);
-EAGER_PTR_EVALUATOR(signed char*, signed char*);
-
// Data for the log message, not stored in LogMessage to avoid increasing the
// stack size.
class LogMessageData;
diff --git a/logging_test.cpp b/logging_test.cpp
index 593e2c1..d8d0855 100644
--- a/logging_test.cpp
+++ b/logging_test.cpp
@@ -22,6 +22,7 @@
#include <signal.h>
#endif
+#include <memory>
#include <regex>
#include <string>
#include <thread>
@@ -66,6 +67,16 @@
ASSERT_DEATH({SuppressAbortUI(); CHECK_EQ(0, 1);}, "Check failed: 0 == 1 ");
CHECK_EQ(0, 0);
+ std::unique_ptr<int> p;
+ ASSERT_DEATH(CHECK_NE(p, nullptr), "Check failed");
+ CHECK_EQ(p, nullptr);
+ CHECK_EQ(p, p);
+
+ const char* kText = "Some text";
+ ASSERT_DEATH(CHECK_NE(kText, kText), "Check failed");
+ ASSERT_DEATH(CHECK_EQ(kText, nullptr), "Check failed.*null");
+ CHECK_EQ(kText, kText);
+
ASSERT_DEATH({SuppressAbortUI(); CHECK_STREQ("foo", "bar");},
R"(Check failed: "foo" == "bar")");
CHECK_STREQ("foo", "foo");
@@ -97,6 +108,13 @@
}
DCHECK_EQ(0, 0);
+ std::unique_ptr<int> p;
+ if (android::base::kEnableDChecks) {
+ ASSERT_DEATH(DCHECK_NE(p, nullptr), "DCheck failed");
+ }
+ DCHECK_EQ(p, nullptr);
+ DCHECK_EQ(p, p);
+
if (android::base::kEnableDChecks) {
ASSERT_DEATH({SuppressAbortUI(); DCHECK_STREQ("foo", "bar");},
R"(DCheck failed: "foo" == "bar")");