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.

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

dgram: prevent disabled optimization of bind() #4613

Closed
wants to merge 1 commit into from
Closed

dgram: prevent disabled optimization of bind() #4613

wants to merge 1 commit into from

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

CI: https://ci.nodejs.org/job/node-test-pull-request/1185/

@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

@mscdex
dgram: prevent disabled optimization of bind()
2ffd365 
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
@mscdex @jasnell
dgram: prevent disabled optimization of bind()
88b2889 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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
@mscdex @rvagg
dgram: prevent disabled optimization of bind()
67d2e2b 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@evanlucas evanlucas mentioned this pull request Jan 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
@mscdex
dgram: prevent disabled optimization of bind()
a97455a 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
@mscdex
dgram: prevent disabled optimization of bind()
cc1fcf4 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
@mscdex
dgram: prevent disabled optimization of bind()
5ad5ebb 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 11, 2016
Merged
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
@mscdex
dgram: prevent disabled optimization of bind()
6e829b4 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@mscdex
dgram: prevent disabled optimization of bind()
c27f545 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
dgram Issues and PRs related to the dgram subsystem / UDP.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mscdex @silverwind @cjihrig @jasnell @MylesBorins