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.

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

Jump to bottom

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
addaleax
addaleax approved these changes Nov 14, 2018
View reviewed changes
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.

/cc @joyeecheung

@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?

@kiyomizumia
src: added check statements for debugging
22ef1c9
@kiyomizumia
src: refactored and removed some ifdef debug statements
4f6fdbd 
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 •
edited
Loading

@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
Closed
2 tasks
joyeecheung
joyeecheung approved these changes Nov 22, 2018
View reviewed changes
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.

joyeecheung
joyeecheung reviewed Nov 22, 2018
View reviewed changes
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

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

@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 joyeecheung mentioned this pull request Nov 28, 2018
Closed
2 tasks
@joyeecheung
Copy link
Member

joyeecheung commented Nov 28, 2018 •
edited
Loading

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!

@joyeecheung @kiyomizumia
Update src/inspector/node_string.h
90de2f3 
Co-Authored-By: kiyomizumia <42000558+kiyomizumia@users.noreply.github.com>
@addaleax
Copy link
Member

addaleax commented Dec 7, 2018

Thanks!

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

@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

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

@Trott
Copy link
Member

Trott commented Dec 24, 2018 •
edited
Loading

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
@kiyomizumia @BridgeAR
src: add DCHECK macros
71bc7e1 
This adds check statements for debugging and refactors the code
accordingly.

PR-URL: nodejs#24359
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@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
@kiyomizumia @MylesBorins
src: add DCHECK macros
e624713 
This adds check statements for debugging and refactors the code
accordingly.

PR-URL: #24359
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Dec 25, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
@kiyomizumia @MylesBorins
src: add DCHECK macros
c9f809e 
This adds check statements for debugging and refactors the code
accordingly.

PR-URL: #24359
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@kiyomizumia @refack
src: add DCHECK macros
2369ace 
This adds check statements for debugging and refactors the code
accordingly.

PR-URL: nodejs#24359
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@addaleax addaleax addaleax approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kiyomizumia @addaleax @joyeecheung @Trott @BridgeAR @cjihrig @BethGriggs @nodejs-github-bot