-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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: improve querystring unescapeBuffer perf #12525
Changes from 1 commit
cbbfbd7
917ed0a
52c4a67
5fd16cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,70 +64,43 @@ const unhexTable = [ | |
// a safe fast alternative to decodeURIComponent | ||
function unescapeBuffer(s, decodeSpaces) { | ||
var out = Buffer.allocUnsafe(s.length); | ||
var state = 0; | ||
var n, m, hexchar, c; | ||
|
||
for (var inIndex = 0, outIndex = 0; ; inIndex++) { | ||
if (inIndex < s.length) { | ||
c = s.charCodeAt(inIndex); | ||
} else { | ||
if (state > 0) { | ||
out[outIndex++] = 37/*%*/; | ||
if (state === 2) | ||
out[outIndex++] = hexchar; | ||
} | ||
break; | ||
var index = 0; | ||
var outIndex = 0; | ||
var c, c1, n, m; | ||
var maxLength = s.length - 2; | ||
// Flag to know if some hex chars have been decoded | ||
var hasHex = false; | ||
while (index < s.length) { | ||
c = s.charCodeAt(index); | ||
if (c === 43 /*'+'*/ && decodeSpaces) { | ||
out[outIndex++] = 32; // ' ' | ||
index++; | ||
continue; | ||
} | ||
switch (state) { | ||
case 0: // Any character | ||
switch (c) { | ||
case 37: // '%' | ||
n = 0; | ||
m = 0; | ||
state = 1; | ||
break; | ||
case 43: // '+' | ||
if (decodeSpaces) | ||
c = 32; // ' ' | ||
// falls through | ||
default: | ||
out[outIndex++] = c; | ||
break; | ||
} | ||
break; | ||
|
||
case 1: // First hex digit | ||
hexchar = c; | ||
n = unhexTable[c]; | ||
if (!(n >= 0)) { | ||
out[outIndex++] = 37/*%*/; | ||
out[outIndex++] = c; | ||
state = 0; | ||
break; | ||
} | ||
state = 2; | ||
break; | ||
|
||
case 2: // Second hex digit | ||
state = 0; | ||
m = unhexTable[c]; | ||
if (!(m >= 0)) { | ||
out[outIndex++] = 37/*%*/; | ||
out[outIndex++] = hexchar; | ||
if (c === 37 /*'%'*/ && index < maxLength) { | ||
c = s.charCodeAt(++index); | ||
n = unhexTable[c]; | ||
if (n < 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This won't work for characters not in the table. This is why I had used |
||
out[outIndex++] = 37; // '%' | ||
} else { | ||
c1 = s.charCodeAt(++index); | ||
m = unhexTable[c1]; | ||
if (m < 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
out[outIndex++] = 37; // '%' | ||
out[outIndex++] = c; | ||
break; | ||
c = c1; | ||
} else { | ||
hasHex = true; | ||
c = n * 16 + m; | ||
} | ||
out[outIndex++] = 16 * n + m; | ||
break; | ||
} | ||
} | ||
out[outIndex++] = c; | ||
index++; | ||
} | ||
|
||
// TODO support returning arbitrary buffers. | ||
|
||
return out.slice(0, outIndex); | ||
return hasHex ? out.slice(0, outIndex) : out; | ||
} | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary whitespace change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the standard? Because the file contains both 2 and 1 line separation between functions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My own opinion is that such changes should be addressed separately. |
||
function qsUnescape(s, decodeSpaces) { | ||
try { | ||
return decodeURIComponent(s); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we name these better and place them on separate lines?