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

querystring: fix up lastPos usage #14151

Closed
wants to merge 1 commit into from

Conversation

TimothyGu
Copy link
Member

Use lastPos ONLY for tracking what has been .slice()'d, never as an
indication of if key/value has been seen, since lastPos is updated on
seeing + as well.

Fixes: #13773

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

querystring

Use lastPos ONLY for tracking what has been .slice()'d, never as an
indication of if key/value has been seen, since lastPos is updated on
seeing + as well.

Fixes: nodejs#13773
@TimothyGu
Copy link
Member Author

@mscdex
Copy link
Contributor

mscdex commented Jul 10, 2017

Did you check benchmarks?

@mscdex
Copy link
Contributor

mscdex commented Jul 10, 2017

Also, why no 'dont-land-on-v6.x' if the bug doesn't exist there?

@TimothyGu
Copy link
Member Author

Benchmark shows nothing of importance:

                                                                 improvement confidence   p.value
 querystring/querystring-parse.js n=300000 type="altspaces"           1.07 %            0.6231257
 querystring/querystring-parse.js n=300000 type="encodefake"          1.07 %            0.4534987
 querystring/querystring-parse.js n=300000 type="encodelast"          5.86 %            0.1839875
 querystring/querystring-parse.js n=300000 type="encodemany"         -1.85 %            0.5168988
 querystring/querystring-parse.js n=300000 type="manyblankpairs"     -3.59 %            0.4018666
 querystring/querystring-parse.js n=300000 type="manypairs"          -1.23 %            0.6901342
 querystring/querystring-parse.js n=300000 type="multicharsep"       -0.26 %            0.8610333
 querystring/querystring-parse.js n=300000 type="multivalue"          4.31 %            0.3000455
 querystring/querystring-parse.js n=300000 type="multivaluemany"     -5.45 %            0.3415254
 querystring/querystring-parse.js n=300000 type="noencode"            1.29 %            0.3334038

Didn't tag dont-land-on-v6.x as I thought this could be helpful just for code readability, not necessarily to fix any bugs in v6.x. But readded the label on a second thought.

@TimothyGu
Copy link
Member Author

@mscdex Any more comments?

@mscdex
Copy link
Contributor

mscdex commented Jul 12, 2017

Not really, LGTM since it passes CI.

@TimothyGu
Copy link
Member Author

@mscdex Thanks.

Landed in afab5ab.

@TimothyGu TimothyGu closed this Jul 12, 2017
@TimothyGu TimothyGu deleted the querystring branch July 12, 2017 04:13
TimothyGu added a commit that referenced this pull request Jul 12, 2017
Use lastPos ONLY for tracking what has been .slice()'d, never as an
indication of if key/value has been seen, since lastPos is updated on
seeing + as well.

PR-URL: #14151
Fixes: #13773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 12, 2017
Use lastPos ONLY for tracking what has been .slice()'d, never as an
indication of if key/value has been seen, since lastPos is updated on
seeing + as well.

PR-URL: #14151
Fixes: #13773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
@addaleax addaleax mentioned this pull request Jul 18, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
Use lastPos ONLY for tracking what has been .slice()'d, never as an
indication of if key/value has been seen, since lastPos is updated on
seeing + as well.

PR-URL: #14151
Fixes: #13773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
Use lastPos ONLY for tracking what has been .slice()'d, never as an
indication of if key/value has been seen, since lastPos is updated on
seeing + as well.

PR-URL: #14151
Fixes: #13773
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
querystring Issues and PRs related to the built-in querystring module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

querystring.parse decodes incorrect in a specific case
4 participants