Skip to content

url: do not use HandleScope in ToObject #14096

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
Closed
wants to merge 1 commit into from
Closed

url: do not use HandleScope in ToObject #14096

wants to merge 1 commit into from

Conversation

@bmeck
Copy link
Member

@bmeck bmeck commented Jul 5, 2017

It is not needed / invalidates the returned value unlike EscapableHandleScope

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

url

@bmeck
url: do not use HandleScope in ToObject
17e3bb7 
It is not needed / invalidates the returned value unlike EscapableHandleScope
@bmeck bmeck requested a review from addaleax July 5, 2017 22:24
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jul 5, 2017
addaleax
addaleax approved these changes Jul 5, 2017
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.

Thanks!

TimothyGu
TimothyGu approved these changes Jul 5, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Is the Context::Scope below necessary?

@bmeck
Copy link
Member Author

bmeck commented Jul 6, 2017

@TimothyGu probably not, unsure though

@TimothyGu
Copy link
Member

TimothyGu commented Jul 6, 2017

@bmeck No matter. This still LGTM.

@bmeck
Copy link
Member Author

bmeck commented Jul 6, 2017

@TimothyGu checked things, and it is unneeded / fixed

@bnoordhuis bnoordhuis mentioned this pull request Jul 6, 2017
Closed
@bnoordhuis
Copy link
Member

bnoordhuis commented Jul 6, 2017

Alternative proposal that simply removes the method: #14106

It's not used anywhere so why keep it around?

@bmeck
Copy link
Member Author

bmeck commented Jul 6, 2017

@bnoordhuis it was added as I need it for ESM

@bnoordhuis
Copy link
Member

bnoordhuis commented Jul 6, 2017

Why was it landed in master instead of your local feature branch? :-S Fixing bugs in dead code is a waste of time.

@bmeck
Copy link
Member Author

bmeck commented Jul 6, 2017

@bnoordhuis fixing bugs is fixing bugs

@bnoordhuis
Copy link
Member

bnoordhuis commented Jul 6, 2017

Very droll. My point stands: if it's not used anywhere, it should go.

@bmeck
Copy link
Member Author

bmeck commented Jul 6, 2017

@bnoordhuis if I PR against my ESM stuff today does it still stand?

@bnoordhuis
Copy link
Member

bnoordhuis commented Jul 6, 2017

If you are about to open a PR, then sure, it makes more sense to fix it than remove it.

@bmeck
Copy link
Member Author

bmeck commented Jul 6, 2017

give me a day or 2 and I will

@XadillaX
Copy link
Contributor

XadillaX commented Jul 7, 2017

LGTM.

@jasnell
Copy link
Member

jasnell commented Jul 10, 2017

The ToObject method should not be removed.

@bmeck
Copy link
Member Author

bmeck commented Jul 17, 2017

@bnoordhuis honestly, if you are going to add to my mental strain right now by this being open, closing it. Do what you want.

@bmeck bmeck closed this Jul 17, 2017
@refack refack reopened this Jul 17, 2017
refack pushed a commit to refack/node that referenced this pull request Jul 17, 2017
@bmeck @refack
url: do not use HandleScope in ToObject
d49e669 
It is not needed / invalidates the returned value unlike EscapableHandleScope

PR-URL: nodejs#14096
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack
Copy link
Contributor

refack commented Jul 17, 2017

Landing this since it only improves the status quo, and does not interfere with #14106

@refack
Copy link
Contributor

refack commented Jul 17, 2017

Landed in d49e669

@refack refack closed this Jul 17, 2017
@addaleax addaleax mentioned this pull request Jul 18, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@bmeck @addaleax
url: do not use HandleScope in ToObject
d23c49f 
It is not needed / invalidates the returned value unlike EscapableHandleScope

PR-URL: #14096
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@bmeck @Fishrock123
url: do not use HandleScope in ToObject
fd17f08 
It is not needed / invalidates the returned value unlike EscapableHandleScope

PR-URL: #14096
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bmeck bmeck mentioned this pull request Jul 26, 2017
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

+2 more reviewers

@refack refack refack approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@bmeck @TimothyGu @bnoordhuis @XadillaX @jasnell @refack @addaleax @cjihrig @nodejs-github-bot