Skip to content

http: remove unnecessary util._extend()#634

Closed
jonathanong wants to merge 1 commit into
nodejs:v1.xfrom
jonathanong:remove-util-extend-http
Closed

http: remove unnecessary util._extend()#634
jonathanong wants to merge 1 commit into
nodejs:v1.xfrom
jonathanong:remove-util-extend-http

Conversation

@jonathanong
Copy link
Copy Markdown
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
Copy Markdown
Contributor

micnic commented Jan 28, 2015

LGTM

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

so no need to use util._extend()

@bnoordhuis
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

But sorting makes a clone...

@bnoordhuis
Copy link
Copy Markdown
Member

Nuh-uh. :-)

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

@jonathanong
Copy link
Copy Markdown
Contributor Author

Waaaaaaa is it because no sorting took place?

@charmander
Copy link
Copy Markdown
Contributor

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

@jonathanong
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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