src: add DCHECK macros by kiyomizumia · Pull Request #24359 · nodejs/node

kiyomizumia · 2018-11-14T08:03:50Z

Refactored and removed some debug #ifdef statements.

addaleax

addaleax · 2018-11-14T14:23:30Z

@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 · 2018-11-16T11:40:15Z

@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 · 2018-11-19T02:54:37Z

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

addaleax · 2018-11-19T10:36:12Z

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

joyeecheung

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

joyeecheung

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

joyeecheung · 2018-11-24T07:25:16Z

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

src/inspector/node_string.h

joyeecheung · 2018-11-28T13:21:12Z

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 · 2018-12-06T22:36:47Z

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 · 2018-12-07T15:33:56Z

@joyeecheung Okay, go ahead!

Co-Authored-By: kiyomizumia <42000558+kiyomizumia@users.noreply.github.com>

addaleax · 2018-12-07T18:01:41Z

Thanks!

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

addaleax · 2018-12-19T20:52:22Z

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

Trott · 2018-12-24T01:13:55Z

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

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 · 2018-12-24T09:45:58Z

Landed in 71bc7e1 🎉

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>

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>

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 approved these changes Nov 14, 2018

View reviewed changes

cjihrig approved these changes Nov 14, 2018

View reviewed changes

kiyomizumia force-pushed the cppcode branch from 53039ba to 602eb98 Compare November 15, 2018 12:19

kiyomizumia added 2 commits November 19, 2018 11:41

src: added check statements for debugging

22ef1c9

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 force-pushed the cppcode branch from 602eb98 to 07c92d3 Compare November 19, 2018 03:20

addaleax mentioned this pull request Nov 22, 2018

src: add DCHECK macros #24563

Closed

2 tasks

joyeecheung approved these changes Nov 22, 2018

View reviewed changes

joyeecheung reviewed Nov 22, 2018

View reviewed changes

src/stream_base.cc Outdated Show resolved Hide resolved

kiyomizumia force-pushed the cppcode branch from 07c92d3 to 4f6fdbd Compare November 24, 2018 06:49

src: fixed DCHECK statements

799e6d5

joyeecheung approved these changes Nov 24, 2018

View reviewed changes

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

src: added ifndef for decheck

2002e03

joyeecheung reviewed Nov 24, 2018

View reviewed changes

src/inspector/node_string.h Show resolved Hide resolved

addaleax reviewed Nov 24, 2018

View reviewed changes

src/inspector/node_string.h Show resolved Hide resolved

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 mentioned this pull request Nov 28, 2018

src: migrate to new V8 array API #24613

Closed

2 tasks

Update src/inspector/node_string.h

90de2f3

Co-Authored-By: kiyomizumia <42000558+kiyomizumia@users.noreply.github.com>

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

joyeecheung changed the title ~~Cppcode~~ src: add DCHECK macros Dec 24, 2018

BridgeAR approved these changes Dec 24, 2018

View reviewed changes

BridgeAR closed this Dec 24, 2018

MylesBorins mentioned this pull request Dec 25, 2018

v11.6.0 proposal #25175

Merged

BethGriggs added the backport-requested-v10.x label Jul 2, 2019

Uh oh!

Conversation

kiyomizumia commented Nov 14, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Nov 14, 2018

Uh oh!

addaleax commented Nov 16, 2018

Uh oh!

kiyomizumia commented Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Nov 19, 2018

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joyeecheung commented Nov 24, 2018

Uh oh!

Uh oh!

Uh oh!

joyeecheung commented Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Dec 6, 2018

Uh oh!

kiyomizumia commented Dec 7, 2018

Uh oh!

addaleax commented Dec 7, 2018

Uh oh!

addaleax commented Dec 19, 2018

Uh oh!

Trott commented Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Dec 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kiyomizumia commented Nov 19, 2018 •

edited

Loading

joyeecheung commented Nov 28, 2018 •

edited

Loading

Trott commented Dec 24, 2018 •

edited

Loading