FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: minor refactor to node_errors.h by addaleax · Pull Request #23879 · nodejs/node · GitHub

/ node Public

src: minor refactor to node_errors.h - #23879

Closed
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:error-isolate
Closed

src: minor refactor to node_errors.h#23879
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:error-isolate

Conversation

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Collaborator

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 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
Comment thread src/node_buffer.cc Outdated
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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Extra space after the comma?

Copy link
Copy Markdown
Member Author

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 25, 2018

targos commented Oct 28, 2018

Copy link
Copy Markdown
Member

Landed in 0fd55e7

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

Copy link
Copy Markdown
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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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 outstanding review comments, 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


Back | FazBrowse Home | New Git URL