Skip to content

Add macro DCHECK in quic_logging_impl.h #6358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alyssawilk merged 10 commits into from
Mar 25, 2019
Merged

Add macro DCHECK in quic_logging_impl.h #6358

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ac872f9
Update quiche dependency.
danzh1989 Mar 20, 2019
717885d
fix use of QuicString
danzh1989 Mar 20, 2019
831bd35
remove logging_impl
danzh1989 Mar 20, 2019
4d4c80d
fix format
danzh1989 Mar 20, 2019
480f8fb
add back quic::
danzh1989 Mar 21, 2019
f3127a2
change to logging.h
danzh1989 Mar 21, 2019
ca4b1d5
implement DCHECK
danzh1989 Mar 21, 2019
c38586b
Merge branch 'master' into dcheck
danzh1989 Mar 22, 2019
0776a2d
move around macro definition
danzh1989 Mar 22, 2019
d5023e3
cosmatic change
danzh1989 Mar 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions source/extensions/quic_listeners/quiche/platform/quic_logging_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@
#include "absl/base/optimization.h"
#include "absl/synchronization/mutex.h"

// TODO(wub): Add CHECK/DCHECK and variants, which are not explicitly exposed by quic_logging.h.

// If |condition| is true, use |logstream| to stream the log message and send it to spdlog.
// If |condition| is false, |logstream| will not be instantiated.
// The switch(0) is used to suppress a compiler warning on ambiguous "else".
Expand Down Expand Up @@ -76,6 +74,17 @@

#define QUIC_PREDICT_FALSE_IMPL(x) ABSL_PREDICT_FALSE(x)

#define CHECK(condition) \

@wu-bin wu-bin Mar 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this CHECK to before the #ifdef at line 56?

@danzh2010 danzh2010 Mar 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

QUIC_LOG_IF_IMPL(FATAL, ABSL_PREDICT_FALSE(!(condition))) << "CHECK failed: " #condition "."

#ifndef NDEBUG
#define DCHECK(condition) CHECK(condition)

@wu-bin wu-bin Mar 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to avoid this #ifndef. Can you move the two #define DCHECK to the existing #ifdef NDEBUG above?

@danzh2010 danzh2010 Mar 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

#else
#define DCHECK(condition) \

@wu-bin wu-bin Mar 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to before line 65 and
#define DCHECK(condition) QUIC_COMPILED_OUT_LOG()

while (false && (condition)) \
quic::NullLogStream().stream()
#endif

namespace quic {

using QuicLogLevel = spdlog::level::level_enum;
Expand Down
9 changes: 9 additions & 0 deletions test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,15 @@ TEST(QuicPlatformTest, QuicDLog) {

#undef VALUE_BY_COMPILE_MODE

TEST(QuicPlatformTest, QuicDCHECK) {

@wu-bin wu-bin Mar 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call it QuicCHECK?

@danzh2010 danzh2010 Mar 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

CHECK(1 == 1);
CHECK(1 == 1) << " 1 == 1 is forever true.";
EXPECT_DEBUG_DEATH({ CHECK(false) << " Supposed to fail in debug mode."; },

@wu-bin wu-bin Mar 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be DCHECK since CHECK fails in release mode. Have you tried to test it with -c opt?

@danzh2010 danzh2010 Mar 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, yes. Added both CHECK and DCHECK here.

"CHECK failed:.* Supposed to fail in debug mode.");

EXPECT_DEBUG_DEATH({ CHECK(false); }, "CHECK failed");

@wu-bin wu-bin Mar 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, should be DCHECK.

}

// Test the behaviors of the cross products of
//
// {QUIC_LOG, QUIC_DLOG} x {FATAL, DFATAL} x {debug, release}
Expand Down