Add macro DCHECK in quic_logging_impl.h#6358
Conversation
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
|
/assign @wu-bin |
wu-bin
left a comment
There was a problem hiding this comment.
Thanks for doing this, just a few small comments.
|
|
||
| #define QUIC_PREDICT_FALSE_IMPL(x) ABSL_PREDICT_FALSE(x) | ||
|
|
||
| #define CHECK(condition) \ |
There was a problem hiding this comment.
Can you move this CHECK to before the #ifdef at line 56?
| #ifndef NDEBUG | ||
| #define DCHECK(condition) CHECK(condition) | ||
| #else | ||
| #define DCHECK(condition) \ |
There was a problem hiding this comment.
Move to before line 65 and
#define DCHECK(condition) QUIC_COMPILED_OUT_LOG()
| QUIC_LOG_IF_IMPL(FATAL, ABSL_PREDICT_FALSE(!(condition))) << "CHECK failed: " #condition "." | ||
|
|
||
| #ifndef NDEBUG | ||
| #define DCHECK(condition) CHECK(condition) |
There was a problem hiding this comment.
I'd prefer to avoid this #ifndef. Can you move the two #define DCHECK to the existing #ifdef NDEBUG above?
|
|
||
| #undef VALUE_BY_COMPILE_MODE | ||
|
|
||
| TEST(QuicPlatformTest, QuicDCHECK) { |
| TEST(QuicPlatformTest, QuicDCHECK) { | ||
| CHECK(1 == 1); | ||
| CHECK(1 == 1) << " 1 == 1 is forever true."; | ||
| EXPECT_DEBUG_DEATH({ CHECK(false) << " Supposed to fail in debug mode."; }, |
There was a problem hiding this comment.
Should be DCHECK since CHECK fails in release mode. Have you tried to test it with -c opt?
There was a problem hiding this comment.
oh, yes. Added both CHECK and DCHECK here.
| EXPECT_DEBUG_DEATH({ CHECK(false) << " Supposed to fail in debug mode."; }, | ||
| "CHECK failed:.* Supposed to fail in debug mode."); | ||
|
|
||
| EXPECT_DEBUG_DEATH({ CHECK(false); }, "CHECK failed"); |
There was a problem hiding this comment.
Same here, should be DCHECK.
Signed-off-by: Dan Zhang <danzh@google.com>
| TEST(QuicPlatformTest, QuicCHECK) { | ||
| CHECK(1 == 1); | ||
| CHECK(1 == 1) << " 1 == 1 is forever true."; | ||
| EXPECT_DEBUG_DEATH({ DCHECK(false) << " Supposed to fail in debug mode."; }, |
There was a problem hiding this comment.
nit, feel free to ignore: It'd be nicer visually if you
- Put the first 2 CHECKs in adjacent lines
- Then followed by an empty line
- Then put the 2 EXPECT_DEBUG_DEATHs in adjacent lines
- Then followed by an empty line
- Then put the 2 EXPECT_DEATHs in adjacent lines
Signed-off-by: Dan Zhang <danzh@google.com>
|
/assign @alyssawilk |
Address one TOTO in that file that (D)CHECK is not explicit listed in platform API, but is supposed to be defined in some impl. Define them in quic_logging_impl.h seems appropriate.
Risk Level: low, not in use
Part of #2557