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

url: fix lint and deopt issues #5300

Closed
wants to merge 1 commit into from
Closed

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Feb 18, 2016

The deopt issues arose from the use of const in specific situations that v8 does not fully support yet.

The deopt issues arose from the use of const in specific situations
that v8 does not fully support yet.
@mscdex mscdex added the url Issues and PRs related to the legacy built-in url module. label Feb 18, 2016
@mscdex
Copy link
Contributor Author

mscdex commented Feb 18, 2016

@evanlucas
Copy link
Contributor

@mscdex can you shed some light on what specifically is not supported to cause the deopt for using const?

@mscdex
Copy link
Contributor Author

mscdex commented Feb 18, 2016

@evanlucas I'm not enough of a v8 guru to understand. At first I thought it had to do with using a ternary expression in the value for const variables, but I've seen it happen when assigning without ternary expressions too. shrug. The actual aborted optimization reason every time though is "Unsupported phi use of const variable."

@evanlucas
Copy link
Contributor

Thanks

@mscdex
Copy link
Contributor Author

mscdex commented Feb 18, 2016

CI is all green again.

@evanlucas
Copy link
Contributor

LGTM

@mscdex
Copy link
Contributor Author

mscdex commented Feb 18, 2016

/cc @Trott @mhdawson

@Trott
Copy link
Member

Trott commented Feb 18, 2016

LGTM. Maybe add Fixes: https://github.com/nodejs/node/issues/5299 in the metadata for the commit message.

@Trott
Copy link
Member

Trott commented Feb 18, 2016

Also, since this fixes CI, I'd be all for landing it in an expedited fashion.

@jasnell
Copy link
Member

jasnell commented Feb 18, 2016

LGTM

@mhdawson
Copy link
Member

@Trott agreed we want to land to fix the CI. Asked James to review since he was one of the original reviewers and since we have his LGTM now I'd say we're ready to land

@mhdawson
Copy link
Member

@Trott do you have cycles to land ? If not I'll try to fit it in this afternoon.

Trott pushed a commit that referenced this pull request Feb 18, 2016
The deopt issues arose from the use of const in specific situations
that v8 does not fully support yet.

Fixes: #5299
PR-URL: #5300
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

Trott commented Feb 18, 2016

Landed in 7d1d3a6

@Trott Trott closed this Feb 18, 2016
@mscdex mscdex deleted the url-fix-lint branch February 19, 2016 17:42
rvagg pushed a commit that referenced this pull request Feb 21, 2016
The deopt issues arose from the use of const in specific situations
that v8 does not fully support yet.

Fixes: #5299
PR-URL: #5300
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins
Copy link
Contributor

this does not land cleanly on LTS, would someone be willing to backport it?

@Trott
Copy link
Member

Trott commented Mar 1, 2016

@thealphanerd I'm guessing it isn't landing cleanly because #4892 has not landed yet. So if that can't land right away, maybe this can also be delayed until that lands first?

/cc @mscdex in case he wants to go ahead and backport. I'm not sure whether there's a reason to push this ahead of that other PR, though.

@MylesBorins
Copy link
Contributor

@Trott good eye... this will land with #4892

@mscdex
Copy link
Contributor Author

mscdex commented Mar 2, 2016

@Trott The only thing to be backported from this PR if the original url perf commit isn't also backported, are merely the few changes made to url.resolveObject() (changing const to var).

@MylesBorins
Copy link
Contributor

I'm moving the label to don't land for right now. We can change it back if #4892 ever lands

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants