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

http: remove unnecessary util._extend() #634

Closed
wants to merge 1 commit into from
Closed

http: remove unnecessary util._extend() #634

wants to merge 1 commit into from

Conversation

jonathanong
Copy link
Contributor

on a mission to remove all instances of util._extend() and I found this. AFAIK, this is unnecessary. 6100228#diff-1c0f1c434b17b7f8795d44a51a14320aR31

/cc @bnoordhuis

@vkurchatkin vkurchatkin added the http Issues or PRs related to the http subsystem. label Jan 28, 2015
@micnic
Copy link
Contributor

micnic commented Jan 28, 2015

LGTM

> Array.isArray(require('_http_common').methods);
true

so no need to use util._extend()

@bnoordhuis
Copy link
Member

The methods array is used inside the http parser guts. That's the reason it's cloned, because accidentally (or intentionally) modifying it is bound to break things. I would just leave it as it is.

@jonathanong
Copy link
Contributor Author

But sorting makes a clone...

@bnoordhuis
Copy link
Member

Nuh-uh. :-)

$ iojs -p 'var a = []; a === a.sort()'
true

@jonathanong
Copy link
Contributor Author

Waaaaaaa is it because no sorting took place?

@charmander
Copy link
Contributor

No, sort always sorts an array in place. .slice might be better than _extend, though.

@jonathanong
Copy link
Contributor Author

@charmander agreed. updated

Qard pushed a commit that referenced this pull request Feb 2, 2015
PR-URL: #634
Reviewed-BY: Nicu Micleușanu <micnic90@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@Qard
Copy link
Member

Qard commented Feb 2, 2015

Landed in 3e67d7e

@Qard Qard closed this Feb 2, 2015
@jonathanong jonathanong deleted the remove-util-extend-http branch February 2, 2015 23:02
@jonathanong
Copy link
Contributor Author

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants