Skip to content

url: do not use HandleScope in ToObject - #14096

Closed
bmeck wants to merge 1 commit into
nodejs:masterfrom
bmeck:url-toobject-no-handlescope
Closed

url: do not use HandleScope in ToObject#14096
bmeck wants to merge 1 commit into
nodejs:masterfrom
bmeck:url-toobject-no-handlescope

Conversation

@bmeck

@bmeckbmeck commented Jul 5, 2017

Copy link
Copy Markdown
Member

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 addaleaxJuly 5, 2017 22:24
@nodejs-github-botnodejs-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

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

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

@TimothyGuTimothyGu left a comment

Copy link
Copy Markdown
Member

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

bmeck commented Jul 6, 2017

Copy link
Copy Markdown
MemberAuthor

@TimothyGu probably not, unsure though

@TimothyGu

TimothyGu commented Jul 6, 2017

Copy link
Copy Markdown
Member

@bmeck No matter. This still LGTM.

@bmeck

bmeck commented Jul 6, 2017

Copy link
Copy Markdown
MemberAuthor

@TimothyGu checked things, and it is unneeded / fixed

@bnoordhuisbnoordhuis mentioned this pull request Jul 6, 2017
Closed
@bnoordhuis

bnoordhuis commented Jul 6, 2017

Copy link
Copy Markdown
Member

Alternative proposal that simply removes the method: #14106

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

@bmeck

bmeck commented Jul 6, 2017

Copy link
Copy Markdown
MemberAuthor

@bnoordhuis it was added as I need it for ESM

@bnoordhuis

bnoordhuis commented Jul 6, 2017

Copy link
Copy Markdown
Member

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

@bmeck

bmeck commented Jul 6, 2017

Copy link
Copy Markdown
MemberAuthor

@bnoordhuis fixing bugs is fixing bugs

@bnoordhuis

bnoordhuis commented Jul 6, 2017

Copy link
Copy Markdown
Member

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

@bmeck

bmeck commented Jul 6, 2017

Copy link
Copy Markdown
MemberAuthor

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

@bnoordhuis

bnoordhuis commented Jul 6, 2017

Copy link
Copy Markdown
Member

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

@bmeck

bmeck commented Jul 6, 2017

Copy link
Copy Markdown
MemberAuthor

give me a day or 2 and I will

@XadillaX

XadillaX commented Jul 7, 2017

Copy link
Copy Markdown
Contributor

LGTM.

@jasnell

jasnell commented Jul 10, 2017

Copy link
Copy Markdown
Member

The ToObject method should not be removed.

@bmeck

bmeck commented Jul 17, 2017

Copy link
Copy Markdown
MemberAuthor

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

@bmeckbmeck closed this Jul 17, 2017
@refackrefack 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

refack commented Jul 17, 2017

Copy link
Copy Markdown
Contributor

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

@refack

refack commented Jul 17, 2017

Copy link
Copy Markdown
Contributor

Landed in d49e669

@refackrefack closed this Jul 17, 2017
@addaleaxaddaleax 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>
@bmeckbmeck mentioned this pull request Jul 26, 2017
Closed
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@cjihrigcjihrigcjihrig approved these changes
+2 more reviewers
@refackrefackrefack approved these changes
@TimothyGuTimothyGuTimothyGu 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-urlIssues 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