-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
src: add DCHECK macros #24563
Conversation
I like the idea. But why not use P.S. I think what I like about DCHECK(Object.Set(value)) |
@refack Isn't that more like an idea for refactoring |
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) But yeah, it's just something to consider... I like the PR anyway. |
I know the PR title isn’t very descript, but I think this has a lot of overlap with #24359 ? |
@addaleax Ah yes, sorry I didn't notice about the ping earlier. I will close this one out. |
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 aifdef 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
make -j4 test
(UNIX), orvcbuild test
(Windows) passes(https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines)