Skip to content

src: add DCHECK macros #24563

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.

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

Closed
wants to merge 2 commits into from
Closed

src: add DCHECK macros #24563

wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

The first commit adds DCHECK macros that are only enabled in debug build and are noops in release buidls.

The second commit refactors StreamBase::CallJSOnreadMethod to use DCHECK instead of a ifdef DEBUG switch.

Reasoning: IMO it's slightly more readable to use DCHECK and it's a utility commonly used in V8 - we can be more generous about adding checks for the debug builds as general assertions to help finding subtle bugs.

Checklist

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 22, 2018
@joyeecheung joyeecheung requested a review from addaleax November 22, 2018 07:03
@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 22, 2018

@refack
Copy link
Contributor

refack commented Nov 22, 2018

I like the idea. But why not use gsl::Expects and gsl::Ensure since they are most probably going to become part of the language? https://en.cppreference.com/w/cpp/language/attributes/contract

P.S. I think what I like about contracts the most is that they are clear about their place (either pre or post conditions) so no one will be tempted to do:

DCHECK(Object.Set(value))

@joyeecheung
Copy link
Member Author

@refack Isn't that more like an idea for refactoring CHECK macros? Also are those features stable enough? Also, I find the documentation very confusing, it reads like someone talking to themsevels: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Ri-expects

@refack
Copy link
Contributor

refack commented Nov 22, 2018

As for stability IIUC they are implemented as close to the new standard as possible (they can even be used by static analysers like clang-tidy)
They can be conditionally turned off (with a Define) with full standard implementation you can define their level (default or audit)
As for documentation at least there is some ¯_(ツ)_/¯

But yeah, it's just something to consider... I like the PR anyway.

@addaleax
Copy link
Member

I know the PR title isn’t very descript, but I think this has a lot of overlap with #24359 ?

@joyeecheung
Copy link
Member Author

@addaleax Ah yes, sorry I didn't notice about the ping earlier. I will close this one out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants