Skip to content
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 #24359

Closed
wants to merge 5 commits into from
Closed

src: add DCHECK macros #24359

wants to merge 5 commits into from

Conversation

kiyomizumia
Copy link
Contributor

Refactored and removed some debug #ifdef statements.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 14, 2018
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

@addaleax
Copy link
Member

@kiyomizumia Do you think you could rebase out the merge commits here, and/or squash the commits together? Our CI doesn’t play too well with merge commits, sadly :/

@addaleax
Copy link
Member

@kiyomizumia I think something went wrong when rebasing here… I think this should work:

$ git checkout cppcode
$ git fetch upstream 
$ git rebase -i upstream/master
# delete all lines except the last 2 ones (your commits)
$ git push --force-with-lease

Could you try that?

test: fixed order of actual and expected arguments

src: ifdef changes

src: refactored and removed some ifdef debug statements
@kiyomizumia
Copy link
Contributor Author

kiyomizumia commented Nov 19, 2018

@addaleax I should remove all of the commits except mine, correct? There are quite a few of them...

@addaleax
Copy link
Member

@kiyomizumia Yes, correct… the last one in this PR (07c92d3) seems to be a bit odd and should probably be left out, but since there are no merge commits anymore, I think we can run CI:

CI: https://ci.nodejs.org/job/node-test-pull-request/18748/

@addaleax addaleax mentioned this pull request Nov 22, 2018
2 tasks
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

CI is happy. Whoever lands this be sure to leave out the first commit.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

BTW it's the last commit that needs to be dropped..also, the first two commits looks a bit odd - the first one not only adds the macro but also drops #ifdef DEBUG in base_object-inl.h without making the CHECKs DCHECK, while the second commit also contains whitespace changes to util.h. I'd suggest either squash them all into one commit during landing, or use some git magic to split into two commits where one only contains changes to util.h and the other one contains other changes to src.

src/stream_base.cc Show resolved Hide resolved
@joyeecheung
Copy link
Member

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 24, 2018
@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 25, 2018
@joyeecheung
Copy link
Member

joyeecheung commented Nov 28, 2018

Ping @kiyomizumia , do you have time to add the missing endif? (I believe you can just click the apply suggestion button in #24359 (comment) instead of making the change locally and pushing again)

@joyeecheung
Copy link
Member

Ping @kiyomizumia again...if you don't mind can I push to the PR branch? I want to get this landed so that DCHECK macros can be used in the code base.

@kiyomizumia
Copy link
Contributor Author

@joyeecheung Okay, go ahead!

@addaleax
Copy link
Member

addaleax commented Dec 7, 2018

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 19, 2018
@addaleax
Copy link
Member

@Trott
Copy link
Member

Trott commented Dec 24, 2018

Resume Build CI; https://ci.nodejs.org/job/node-test-pull-request/19778/ ✔️

@joyeecheung joyeecheung changed the title Cppcode src: add DCHECK macros Dec 24, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Dec 24, 2018
This adds check statements for debugging and refactors the code
accordingly.

PR-URL: nodejs#24359
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR
Copy link
Member

Landed in 71bc7e1 🎉

@BridgeAR BridgeAR closed this Dec 24, 2018
MylesBorins pushed a commit that referenced this pull request Dec 25, 2018
This adds check statements for debugging and refactors the code
accordingly.

PR-URL: #24359
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 25, 2018
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
This adds check statements for debugging and refactors the code
accordingly.

PR-URL: #24359
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
This adds check statements for debugging and refactors the code
accordingly.

PR-URL: nodejs#24359
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants