Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@mscdex
Copy link
Contributor

@mscdex mscdex commented Jan 10, 2016

Reassigning a named parameter while also using the arguments object causes the entire function to never be optimized.

@mscdex mscdex added the dgram Issues and PRs related to the dgram subsystem / UDP. label Jan 10, 2016
@mscdex
Copy link
Contributor Author

mscdex commented Jan 10, 2016

@silverwind
Copy link
Contributor

Have you thought about refactoring it to not use arguments? Might be a cleaner solution.

@mscdex
Copy link
Contributor Author

mscdex commented Jan 11, 2016

I went for minimal impact on this one. shrug

@silverwind
Copy link
Contributor

Well, LGTM either way.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 11, 2016

LGTM, but would it be cleaner to just do let port = port_; before you start referencing it? Or maybe even let port = arguments[0];

@mscdex
Copy link
Contributor Author

mscdex commented Jan 11, 2016

@cjihrig port is also used inside the lookup callback below the modified code, wouldn't let prevent access to port from there?

@cjihrig
Copy link
Contributor

cjihrig commented Jan 11, 2016

I don't think it should make a difference in that case.

@jasnell
Copy link
Member

jasnell commented Jan 11, 2016

LGTM if CI is green

Reassigning a named parameter while also using the arguments
object causes the entire function to never be optimized.
@mscdex mscdex force-pushed the fix-disabled-opt-dgram-bind branch from a2b4c49 to 2ffd365 Compare January 13, 2016 08:03
@mscdex
Copy link
Contributor Author

mscdex commented Jan 13, 2016

Updated PR to simplify things as suggested. CI again for fun: https://ci.nodejs.org/job/node-test-pull-request/1216/

@silverwind
Copy link
Contributor

Ah, much better. LGTM.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 13, 2016

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented Jan 13, 2016

LGTM

jasnell pushed a commit that referenced this pull request Jan 13, 2016
Reassigning a named parameter while also using the arguments
object causes the entire function to never be optimized.

PR-URL: #4613
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jan 13, 2016

Landed in 88b2889

@jasnell jasnell closed this Jan 13, 2016
@mscdex mscdex deleted the fix-disabled-opt-dgram-bind branch January 13, 2016 17:56
rvagg pushed a commit that referenced this pull request Jan 14, 2016
Reassigning a named parameter while also using the arguments
object causes the entire function to never be optimized.

PR-URL: #4613
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
Reassigning a named parameter while also using the arguments
object causes the entire function to never be optimized.

PR-URL: #4613
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
Reassigning a named parameter while also using the arguments
object causes the entire function to never be optimized.

PR-URL: #4613
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
Reassigning a named parameter while also using the arguments
object causes the entire function to never be optimized.

PR-URL: nodejs#4613
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 11, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
Reassigning a named parameter while also using the arguments
object causes the entire function to never be optimized.

PR-URL: nodejs#4613
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Reassigning a named parameter while also using the arguments
object causes the entire function to never be optimized.

PR-URL: nodejs#4613
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dgram Issues and PRs related to the dgram subsystem / UDP.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.