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: minor refactor to node_errors.h #23879

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Add overloads of the error generation/throwing methods
that take an Isolate* argument, since the created objects
don’t depend on the Environment* in question.

Also, remove THROW_ERR_OUT_OF_RANGE_WITH_TEXT, which did the
same thing as THROW_ERR_OUT_OF_RANGE in a more convoluted way.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Add overloads of the error generation/throwing methods
that take an `Isolate*` argument, since the created objects
don’t depend on the `Environment*` in question.

Also, remove `THROW_ERR_OUT_OF_RANGE_WITH_TEXT`, which did the
same thing as `THROW_ERR_OUT_OF_RANGE` in a more convoluted way.
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Oct 25, 2018
@addaleax addaleax added errors Issues and PRs related to JavaScript errors originated in Node.js core. and removed buffer Issues and PRs related to the buffer subsystem. labels Oct 25, 2018
@@ -41,8 +41,7 @@
#define THROW_AND_RETURN_IF_OOB(r) \
do { \
if (!(r)) \
return node::THROW_ERR_OUT_OF_RANGE_WITH_TEXT(env, \
"Index out of range"); \
return node::THROW_ERR_OUT_OF_RANGE(env, "Index out of range"); \
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra space after the comma?

@addaleax
Copy link
Member Author

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

targos commented Oct 28, 2018

Landed in 0fd55e7

@targos targos closed this Oct 28, 2018
targos pushed a commit that referenced this pull request Oct 28, 2018
Add overloads of the error generation/throwing methods
that take an `Isolate*` argument, since the created objects
don’t depend on the `Environment*` in question.

Also, remove `THROW_ERR_OUT_OF_RANGE_WITH_TEXT`, which did the
same thing as `THROW_ERR_OUT_OF_RANGE` in a more convoluted way.

PR-URL: #23879
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
targos pushed a commit that referenced this pull request Oct 28, 2018
Add overloads of the error generation/throwing methods
that take an `Isolate*` argument, since the created objects
don’t depend on the `Environment*` in question.

Also, remove `THROW_ERR_OUT_OF_RANGE_WITH_TEXT`, which did the
same thing as `THROW_ERR_OUT_OF_RANGE` in a more convoluted way.

PR-URL: #23879
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@addaleax addaleax deleted the error-isolate branch October 28, 2018 14:34
targos pushed a commit that referenced this pull request Nov 1, 2018
Add overloads of the error generation/throwing methods
that take an `Isolate*` argument, since the created objects
don’t depend on the `Environment*` in question.

Also, remove `THROW_ERR_OUT_OF_RANGE_WITH_TEXT`, which did the
same thing as `THROW_ERR_OUT_OF_RANGE` in a more convoluted way.

PR-URL: #23879
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@MylesBorins
Copy link
Contributor

This does not land cleanly in v10.x or v8.x LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

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++. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants