ARROW-6781: [C++] Improve and consolidate CHECK macros #8735

bkietz · 2020-11-21T20:45:32Z

Adds more descriptive failures for comparison DCHECKs when ARROW_EXTRA_ERROR_CONTEXT is defined.

  DCHECK(vec.size() == expected_size) << "fails and prints operands";
  // ../src/arrow/util/logging_test.cc:101:  Check failed: (vec.size() == expected_size)
  //   left:  3
  //   right: 0
  // fails and prints operands

  DCHECK(std::vector<int>{1, 2} == std::vector<int>{})
      << "vector isn't printable - no extra printing";

  DCHECK_OK(Status::OK()) << "Status Checks can be streamed into";

Note that the above destructuring evaluates operands twice. This is fine for debug checks which must be constant anyway to ensure behavior does not differ between release/debug. However ARROW_CHECK might be side-effectful (for example: ARROW_CHECK(PokeServer(url).successful())). Thus if the extra context is desired there as well we'd first need to forbid side-effectful ARROW_CHECKs; not sure it's worth the trouble.

At the same time we could simply remove DCHECK_(EQ|NE|LE|LT|GE|GT): IMHO they add no value. In any case that cleanup is very noisy and can wait until the smaller core of this patch is reviewed.

github-actions · 2020-11-21T20:47:30Z

https://issues.apache.org/jira/browse/ARROW-6781

pitrou · 2020-11-23T13:25:51Z

This PR needs rebasing now.

pitrou

There are a couple oddities here.

pitrou · 2020-11-24T15:29:12Z

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)


Is there a reason this is commented out?

Yes, this is a lint check which fails for DCHECK(x == y) (consider using DCHECK_EQ(x, y))

pitrou · 2020-11-24T15:33:11Z

cpp/src/arrow/util/logging.h

+#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)


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)?

okay, but since these are debug checks anyway the giving them side effects results in differing behavior between release and debug. In general it's buggy to have non trivial code inside a debug check

I'm not saying the comparison would have a side-effect, just that the compiler wouldn't know about it and therefore couldn't entirely eliminate the comparison code.

Ah, I see. In that case I should prefix if (false) so the entire expr can be dropped

pitrou · 2020-11-24T15:36:07Z

cpp/src/arrow/util/logging.h

+// 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__)); \


Is there a particular reason for using a for loop?

Yes, it allows us to confine _s to the scope of the loop body. The loop body is a single statement with no braces, allowing *CHECK_OK() to accept a stream.

Ok, but I mean compared to a regular while or do...while?

The last tokens of a do... while loop are the loop condition, which precludes providing a stream for extra error messages.

for supports a scoped decl, which while doesn't. If we wanted to avoid for, we could introduce a variable as in ARROW_ASSIGN_OR_RAISE. After some macro expansion that'd look like:

Status _s_56 = Thing(); if (ARROW_PREDICT_FALSE(!_s_56.ok())) ARROW_LOG(FATAL) << _s.message() << "Extra messages";

It seems more desirable to keep things scoped with a for

pitrou · 2020-11-24T15:39:56Z

cpp/src/arrow/util/logging.h

-namespace detail {
+  template <typename Lhs, typename Rhs>
+  struct BoundLhsRhs {
+    explicit constexpr operator bool() const { return false; }


Hmm... why?

These bound expressions must be contextually convertible to bool to enable DCHECK(x >= 0 && x < 8). (It can't be destructured for better printing, but it should still be a valid assertion.) If the conversion to bool is not provided, then bound_expr && x < 8 breaks compilation

Would there be a less cryptic way to implement the whole thing?

pitrou · 2020-11-24T15:40:19Z

cpp/src/arrow/util/logging.h

+    template <typename Rhs>
+    BoundLhsRhs<Lhs, Rhs> operator<=(const Rhs& rhs) && {
+      return {lhs, rhs};
+    }


Why all these operators? They don't look like they perform the advertised operations.

The assert condition is tested without decoration. This avoids some compiler warnings and suspicion that destructuring might slow down the fast path (the check passes).

Destructuring operates on another copy of the condition expression for printing purposes only.

It relies on the slightly higher precedence of <= compared to the other comparison operators. After macro expansion, the intercepted comparison is as follows:

InterceptComparison() <= x > y ((InterceptComparison() <= x) > y) // parentheses for clarity

The inner parenthesis captures x, then the outer parenthesis captures y

Same question then :-)

This is the approach used by the Catch2 test framework's assertion macro. I'm not aware of another way to destructure a comparison's operands

Ok, but destructuring in the first place shouldn't be required. I guess you want DCHECK(a == b) to function exactly like DCHECK_EQ(a, b), but that doesn't sound required to me.

Otherwise, please at least explain all this in comments, because it's extremely cryptic at first sight.

pitrou · 2020-11-24T15:41:14Z

cpp/src/arrow/util/logging.h

+  };
+
+  template <typename Lhs>
+  BoundLhs<Lhs> operator<=(const Lhs& lhs) && {


Same question here... Don't you want to used regular method names?

bkietz · 2021-01-04T17:30:20Z

closing for now

bkietz requested a review from emkornfield November 21, 2020 20:45

github-actions bot added Component: FlightRPC Component: C++ - Gandiva Component: C++ Component: Parquet labels Nov 21, 2020

bkietz force-pushed the 6781-Improve-and-consolidate-A.2 branch from a169b65 to ab3a157 Compare November 23, 2020 02:03

pitrou changed the title ~~ARROW-6781: [C++] Improve and consoldiate CHECK macros~~ ARROW-6781: [C++] Improve and consolidate CHECK macros Nov 23, 2020

bkietz added 10 commits November 23, 2020 09:26

ARROW-6781: [C++] Improve and consoldiate CHECK macros

0252b24

delete *CHECK_NE

fe86cf3

disable CheckCheck lint pass

9716250

delete *CHECK_EQ

9b311bd

test raw comparison rather than captured operands

45b375e

use __VA_ARGS__ for variadic macros

caeb9b1

confine comparison decomposition to EXTRA_ERROR_CONTEXT

368ff3c

revert delete CHECK_EQ

487d571

revert delete *CHECK_NE

91e3a69

revert glog change until follow up

ed294f1

bkietz force-pushed the 6781-Improve-and-consolidate-A.2 branch from 1dd33a4 to 9434bf1 Compare November 23, 2020 14:40

ubsan: prevent pointer arithmetic on nullptr

ecb31b2

bkietz force-pushed the 6781-Improve-and-consolidate-A.2 branch from 9434bf1 to ecb31b2 Compare November 23, 2020 14:53

pitrou reviewed Nov 24, 2020

View reviewed changes

github-actions bot added the needs-rebase A PR that needs to be rebased by the author label Dec 1, 2020

bkietz closed this Jan 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-6781: [C++] Improve and consolidate CHECK macros #8735

ARROW-6781: [C++] Improve and consolidate CHECK macros #8735

bkietz commented Nov 21, 2020 •

edited

github-actions bot commented Nov 21, 2020

pitrou commented Nov 23, 2020

pitrou left a comment

pitrou Nov 24, 2020

bkietz Nov 24, 2020

pitrou Nov 24, 2020

bkietz Nov 24, 2020

pitrou Nov 25, 2020

bkietz Nov 25, 2020

pitrou Nov 24, 2020

bkietz Nov 24, 2020

pitrou Nov 25, 2020

bkietz Nov 25, 2020 •

edited

pitrou Nov 24, 2020

bkietz Nov 24, 2020

pitrou Nov 25, 2020

pitrou Nov 24, 2020

bkietz Nov 24, 2020

pitrou Nov 25, 2020

bkietz Nov 25, 2020

pitrou Nov 25, 2020

pitrou Nov 24, 2020

bkietz commented Jan 4, 2021

ARROW-6781: [C++] Improve and consolidate CHECK macros #8735

ARROW-6781: [C++] Improve and consolidate CHECK macros #8735

Conversation

bkietz commented Nov 21, 2020 • edited

github-actions bot commented Nov 21, 2020

pitrou commented Nov 23, 2020

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkietz Nov 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkietz commented Jan 4, 2021

bkietz commented Nov 21, 2020 •

edited

bkietz Nov 25, 2020 •

edited