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: replace assert with CHECK_LE in node_api.cc #14514

Merged
merged 2 commits into from Aug 7, 2017

Conversation

@bnoordhuis
Member

bnoordhuis commented Jul 27, 2017

@TimothyGu

Actually, I'm going to have renege on my LGTM earlier. This file is AFAIK shared with an outside project that don't have access to the internal CHECK macros. I remember @gabrielschulhof said this might not be true any more after #14217 (comment), but I'd like to block this first before confirmation that such change is acceptable.

/cc @nodejs/n-api

@jasongin

This comment has been minimized.

Show comment
Hide comment
@jasongin

jasongin Aug 1, 2017

Contributor

@TimothyGu Actually the CHECK macros are OK to use now in node_api.cc. They are defined for the node-addon-api project at https://github.com/nodejs/node-addon-api/blob/master/src/node_internals.h#L33

Contributor

jasongin commented Aug 1, 2017

@TimothyGu Actually the CHECK macros are OK to use now in node_api.cc. They are defined for the node-addon-api project at https://github.com/nodejs/node-addon-api/blob/master/src/node_internals.h#L33

@refack

refack approved these changes Aug 2, 2017

bnoordhuis added some commits Jul 27, 2017

src: replace assert with CHECK_LE in node_api.cc
PR-URL: #14514
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nie�en <tniessen@tnie.de>
src: remove unused Connection::ClearError()
PR-URL: #14514
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nie�en <tniessen@tnie.de>

@bnoordhuis bnoordhuis closed this Aug 7, 2017

@bnoordhuis bnoordhuis deleted the bnoordhuis:ndebug branch Aug 7, 2017

@bnoordhuis bnoordhuis merged commit e658d32 into nodejs:master Aug 7, 2017

addaleax added a commit that referenced this pull request Aug 7, 2017

src: replace assert with CHECK_LE in node_api.cc
PR-URL: #14514
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nie�en <tniessen@tnie.de>

addaleax added a commit that referenced this pull request Aug 7, 2017

src: remove unused Connection::ClearError()
PR-URL: #14514
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nie�en <tniessen@tnie.de>

icarter09 added a commit to icarter09/node that referenced this pull request Aug 12, 2017

src: replace assert with CHECK_LE in node_api.cc
PR-URL: nodejs#14514
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nie�en <tniessen@tnie.de>

icarter09 added a commit to icarter09/node that referenced this pull request Aug 12, 2017

src: remove unused Connection::ClearError()
PR-URL: nodejs#14514
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nie�en <tniessen@tnie.de>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins
Member

MylesBorins commented Aug 16, 2017

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 16, 2017

Member

Won't hurt.

Member

bnoordhuis commented Aug 16, 2017

Won't hurt.

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 19, 2017

Member

only one of these two changes apply to the branch. for the sake of simplicity I'm going to label this do not land.

Member

MylesBorins commented Sep 19, 2017

only one of these two changes apply to the branch. for the sake of simplicity I'm going to label this do not land.

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 12, 2018

src: replace assert with CHECK_LE in node_api.cc
PR-URL: nodejs#14514
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nie�en <tniessen@tnie.de>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 15, 2018

src: replace assert with CHECK_LE in node_api.cc
PR-URL: nodejs#14514
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nie�en <tniessen@tnie.de>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 3, 2018

src: replace assert with CHECK_LE in node_api.cc
PR-URL: nodejs#14514
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nie�en <tniessen@tnie.de>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 6, 2018

src: replace assert with CHECK_LE in node_api.cc
PR-URL: nodejs#14514
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nie�en <tniessen@tnie.de>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018

src: replace assert with CHECK_LE in node_api.cc
PR-URL: nodejs#14514
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nie�en <tniessen@tnie.de>

MylesBorins added a commit that referenced this pull request Apr 16, 2018

src: replace assert with CHECK_LE in node_api.cc
Backport-PR-URL: #19447
PR-URL: #14514
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nie�en <tniessen@tnie.de>

@MylesBorins MylesBorins referenced this pull request Apr 16, 2018

Merged

v6.14.2 proposal #19996

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment