You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/11/24 15:42:51 UTC
[GitHub] [arrow] pitrou commented on a change in pull request #8735: ARROW-6781: [C++] Improve and consolidate CHECK macros
pitrou commented on a change in pull request #8735:
URL: https://github.com/apache/arrow/pull/8735#discussion_r529636621
##########
File path: cpp/build-support/cpplint.py
##########
@@ -4560,7 +4560,7 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state,
CheckCommaSpacing(filename, clean_lines, linenum, error)
CheckBracesSpacing(filename, clean_lines, linenum, nesting_state, error)
CheckSpacingForFunctionCall(filename, clean_lines, linenum, error)
- CheckCheck(filename, clean_lines, linenum, error)
+ #CheckCheck(filename, clean_lines, linenum, error)
Review comment:
Is there a reason this is commented out?
##########
File path: cpp/src/arrow/util/logging.h
##########
@@ -86,56 +67,40 @@ enum class ArrowLogLevel : int {
#define ARROW_CHECK_GE(val1, val2) ARROW_CHECK((val1) >= (val2))
#define ARROW_CHECK_GT(val1, val2) ARROW_CHECK((val1) > (val2))
-#ifdef NDEBUG
-#define ARROW_DFATAL ::arrow::util::ArrowLogLevel::ARROW_WARNING
+#define ARROW_LOG_FAILED_STATUS(status, ...) \
+ ARROW_LOG(FATAL) << " Operation failed: " << ARROW_STRINGIFY(__VA_ARGS__) << "\n" \
+ << " Bad status: " << _s.ToString()
+
+// If the status is bad, CHECK immediately, appending the status to the logged
+// message.
+#define ARROW_CHECK_OK(...) \
+ for (::arrow::Status _s = ::arrow::internal::GenericToStatus((__VA_ARGS__)); \
Review comment:
Is there a particular reason for using a for loop?
##########
File path: cpp/src/arrow/util/logging.h
##########
@@ -22,22 +22,21 @@
// The LLVM IR code doesn't have an NDEBUG mode. And, it shouldn't include references to
// streams or stdc++. So, making the DCHECK calls void in that case.
-#define ARROW_IGNORE_EXPR(expr) ((void)(expr))
-
-#define DCHECK(condition) ARROW_IGNORE_EXPR(condition)
-#define DCHECK_OK(status) ARROW_IGNORE_EXPR(status)
-#define DCHECK_EQ(val1, val2) ARROW_IGNORE_EXPR(val1)
-#define DCHECK_NE(val1, val2) ARROW_IGNORE_EXPR(val1)
-#define DCHECK_LE(val1, val2) ARROW_IGNORE_EXPR(val1)
-#define DCHECK_LT(val1, val2) ARROW_IGNORE_EXPR(val1)
-#define DCHECK_GE(val1, val2) ARROW_IGNORE_EXPR(val1)
-#define DCHECK_GT(val1, val2) ARROW_IGNORE_EXPR(val1)
+#define DCHECK(condition) ARROW_UNUSED(condition)
+#define DCHECK_OK(status) ARROW_UNUSED(status)
+#define DCHECK_EQ(val1, val2) ARROW_UNUSED(val1 == val2)
+#define DCHECK_NE(val1, val2) ARROW_UNUSED(val1 != val2)
+#define DCHECK_LE(val1, val2) ARROW_UNUSED(val1 <= val2)
+#define DCHECK_LT(val1, val2) ARROW_UNUSED(val1 < val2)
+#define DCHECK_GE(val1, val2) ARROW_UNUSED(val1 >= val2)
+#define DCHECK_GT(val1, val2) ARROW_UNUSED(val1 > val2)
Review comment:
Hmm... if the comparisons are not trivial (for example they call a function that the compiler can't see whether it has side effects), then not all code will be eliminated?
Perahsp instead something like `ARROW_UNUSED(val1), ARROW_UNUSED(val2)`?
##########
File path: cpp/src/arrow/util/logging.h
##########
@@ -202,48 +169,81 @@ class ARROW_EXPORT ArrowLog : public ArrowLogBase {
private:
ARROW_DISALLOW_COPY_AND_ASSIGN(ArrowLog);
+ std::ostream* Stream() override;
+
// Hide the implementation of log provider by void *.
// Otherwise, lib user may define the same macro to use the correct header file.
void* logging_provider_;
/// True if log messages should be logged and false if they should be ignored.
bool is_enabled_;
static ArrowLogLevel severity_threshold_;
-
- protected:
- std::ostream& Stream() override;
};
-// This class make ARROW_CHECK compilation pass to change the << operator to void.
-// This class is copied from glog.
-class ARROW_EXPORT Voidify {
- public:
- Voidify() {}
- // This has to be an operator with a precedence lower than << but
- // higher than ?:
- void operator&(ArrowLogBase&) {}
-};
+struct ARROW_EXPORT InterceptComparison {
+ template <typename NotBinaryOrNotPrintable>
+ static ArrowLogBase&& PrintOperands(const NotBinaryOrNotPrintable&,
+ ArrowLogBase&& log) {
+ return std::move(log);
+ }
-namespace detail {
+ template <typename Lhs, typename Rhs>
+ struct BoundLhsRhs {
+ explicit constexpr operator bool() const { return false; }
Review comment:
Hmm... why?
##########
File path: cpp/src/arrow/util/logging.h
##########
@@ -202,48 +169,81 @@ class ARROW_EXPORT ArrowLog : public ArrowLogBase {
private:
ARROW_DISALLOW_COPY_AND_ASSIGN(ArrowLog);
+ std::ostream* Stream() override;
+
// Hide the implementation of log provider by void *.
// Otherwise, lib user may define the same macro to use the correct header file.
void* logging_provider_;
/// True if log messages should be logged and false if they should be ignored.
bool is_enabled_;
static ArrowLogLevel severity_threshold_;
-
- protected:
- std::ostream& Stream() override;
};
-// This class make ARROW_CHECK compilation pass to change the << operator to void.
-// This class is copied from glog.
-class ARROW_EXPORT Voidify {
- public:
- Voidify() {}
- // This has to be an operator with a precedence lower than << but
- // higher than ?:
- void operator&(ArrowLogBase&) {}
-};
+struct ARROW_EXPORT InterceptComparison {
+ template <typename NotBinaryOrNotPrintable>
+ static ArrowLogBase&& PrintOperands(const NotBinaryOrNotPrintable&,
+ ArrowLogBase&& log) {
+ return std::move(log);
+ }
-namespace detail {
+ template <typename Lhs, typename Rhs>
+ struct BoundLhsRhs {
+ explicit constexpr operator bool() const { return false; }
+ const Lhs& lhs;
+ const Rhs& rhs;
+ };
+
+ template <typename Lhs, typename Rhs>
+ static auto PrintOperands(const BoundLhsRhs<Lhs, Rhs>& bound, ArrowLogBase&& log)
+ -> decltype(std::move(log) << bound.lhs << bound.rhs) {
+ return std::move(log) << "\n left: " << bound.lhs << "\n right: " << bound.rhs
+ << "\n";
+ }
-/// @brief A helper for the nil log sink.
-///
-/// Using this helper is analogous to sending log messages to /dev/null:
-/// nothing gets logged.
-class NullLog {
- public:
- /// The no-op output operator.
- ///
- /// @param [in] t
- /// The object to send into the nil sink.
- /// @return Reference to the updated object.
- template <class T>
- NullLog& operator<<(const T& t) {
- return *this;
+ template <typename Lhs>
+ struct BoundLhs {
+ template <typename Rhs>
+ BoundLhsRhs<Lhs, Rhs> operator==(const Rhs& rhs) && {
+ return {lhs, rhs};
+ }
+
+ template <typename Rhs>
+ BoundLhsRhs<Lhs, Rhs> operator!=(const Rhs& rhs) && {
+ return {lhs, rhs};
+ }
+
+ template <typename Rhs>
+ BoundLhsRhs<Lhs, Rhs> operator>(const Rhs& rhs) && {
+ return {lhs, rhs};
+ }
+
+ template <typename Rhs>
+ BoundLhsRhs<Lhs, Rhs> operator>=(const Rhs& rhs) && {
+ return {lhs, rhs};
+ }
+
+ template <typename Rhs>
+ BoundLhsRhs<Lhs, Rhs> operator<(const Rhs& rhs) && {
+ return {lhs, rhs};
+ }
+
+ template <typename Rhs>
+ BoundLhsRhs<Lhs, Rhs> operator<=(const Rhs& rhs) && {
+ return {lhs, rhs};
+ }
+
+ explicit constexpr operator bool() const { return false; }
+
+ const Lhs& lhs;
+ };
+
+ template <typename Lhs>
+ BoundLhs<Lhs> operator<=(const Lhs& lhs) && {
Review comment:
Same question here... Don't you want to used regular method names?
##########
File path: cpp/src/arrow/util/logging.h
##########
@@ -202,48 +169,81 @@ class ARROW_EXPORT ArrowLog : public ArrowLogBase {
private:
ARROW_DISALLOW_COPY_AND_ASSIGN(ArrowLog);
+ std::ostream* Stream() override;
+
// Hide the implementation of log provider by void *.
// Otherwise, lib user may define the same macro to use the correct header file.
void* logging_provider_;
/// True if log messages should be logged and false if they should be ignored.
bool is_enabled_;
static ArrowLogLevel severity_threshold_;
-
- protected:
- std::ostream& Stream() override;
};
-// This class make ARROW_CHECK compilation pass to change the << operator to void.
-// This class is copied from glog.
-class ARROW_EXPORT Voidify {
- public:
- Voidify() {}
- // This has to be an operator with a precedence lower than << but
- // higher than ?:
- void operator&(ArrowLogBase&) {}
-};
+struct ARROW_EXPORT InterceptComparison {
+ template <typename NotBinaryOrNotPrintable>
+ static ArrowLogBase&& PrintOperands(const NotBinaryOrNotPrintable&,
+ ArrowLogBase&& log) {
+ return std::move(log);
+ }
-namespace detail {
+ template <typename Lhs, typename Rhs>
+ struct BoundLhsRhs {
+ explicit constexpr operator bool() const { return false; }
+ const Lhs& lhs;
+ const Rhs& rhs;
+ };
+
+ template <typename Lhs, typename Rhs>
+ static auto PrintOperands(const BoundLhsRhs<Lhs, Rhs>& bound, ArrowLogBase&& log)
+ -> decltype(std::move(log) << bound.lhs << bound.rhs) {
+ return std::move(log) << "\n left: " << bound.lhs << "\n right: " << bound.rhs
+ << "\n";
+ }
-/// @brief A helper for the nil log sink.
-///
-/// Using this helper is analogous to sending log messages to /dev/null:
-/// nothing gets logged.
-class NullLog {
- public:
- /// The no-op output operator.
- ///
- /// @param [in] t
- /// The object to send into the nil sink.
- /// @return Reference to the updated object.
- template <class T>
- NullLog& operator<<(const T& t) {
- return *this;
+ template <typename Lhs>
+ struct BoundLhs {
+ template <typename Rhs>
+ BoundLhsRhs<Lhs, Rhs> operator==(const Rhs& rhs) && {
+ return {lhs, rhs};
+ }
+
+ template <typename Rhs>
+ BoundLhsRhs<Lhs, Rhs> operator!=(const Rhs& rhs) && {
+ return {lhs, rhs};
+ }
+
+ template <typename Rhs>
+ BoundLhsRhs<Lhs, Rhs> operator>(const Rhs& rhs) && {
+ return {lhs, rhs};
+ }
+
+ template <typename Rhs>
+ BoundLhsRhs<Lhs, Rhs> operator>=(const Rhs& rhs) && {
+ return {lhs, rhs};
+ }
+
+ template <typename Rhs>
+ BoundLhsRhs<Lhs, Rhs> operator<(const Rhs& rhs) && {
+ return {lhs, rhs};
+ }
+
+ template <typename Rhs>
+ BoundLhsRhs<Lhs, Rhs> operator<=(const Rhs& rhs) && {
+ return {lhs, rhs};
+ }
Review comment:
Why all these operators? They don't look like they perform the advertised operations.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org