Use higher-level string helpers where possible. NFC#24486
Use higher-level string helpers where possible. NFC#24486RReverser merged 5 commits intoemscripten-core:mainfrom
Conversation
| for (var i = 0; i < data.length; i++) { | ||
| {{{ makeSetValue('dataBuffer', 'i', 'data[i]', 'i8') }}}; | ||
| if (typeof data == 'string') { | ||
| len = stringToUTF8(data, dataBuffer, len); |
There was a problem hiding this comment.
Does len need to be re-assigned here? it looks like it will remain the same.
There was a problem hiding this comment.
It will be len - 1 as before the call it's length with nul character, but afterwards we just want the length of the string itself.
So it's either reassignment or doing len-- manually, I figured reassignment is cleaner.
| if (channelInfo.audio === this || channelInfo.audio.webAudioNode === this) { | ||
| channelInfo.audio.paused = true; channelInfo.audio = null; | ||
| if (channelInfo.audio === this || channelInfo.audio.webAudioNode === this) { | ||
| channelInfo.audio.paused = true; channelInfo.audio = null; |
There was a problem hiding this comment.
I find these types of cleanups, when included in other changes, to be distracting during review, so in general I'd rather see them as separate PRs (no need to split up this one though).
BTW, I'm sure this is old new to you, but I recommend configuring a keyboard shortcut to trim whitespace in the whole file, rather than doing to automatically on all files you save.. that way you don't end up creating changes like these when working on projects that don't have good whitespace hygiene.
There was a problem hiding this comment.
I'm sure this is old new to you, but I recommend configuring a keyboard shortcut to trim whitespace in the whole file, rather than doing to automatically on all files you save
Heh I remember we talked about that a while back. It's just extremely rare for me to encounter files with trailing spaces, so having it done automatically on save helps more than hurts, as it usually just removes spaces I introduced myself, but then there are exceptions like this one.
FWIW I almost always review Github diffs with "ignore whitespace" checked, it helps with the distraction aspect. But if you want, I can revert this one.
|
|
||
| function fetchGetResponseHeadersLength(id) { | ||
| return lengthBytesUTF8(Fetch.xhrs.get(id).getAllResponseHeaders()) + 1; | ||
| return lengthBytesUTF8(Fetch.xhrs.get(id).getAllResponseHeaders()); |
There was a problem hiding this comment.
Doesn't this change the semantics of the function? i.e. don't we want to include the null terminator in the size of the string here?
There was a problem hiding this comment.
We don't, but yeah it does - I mentioned it in the PR description.
| var lengthBytes = lengthBytesUTF8(responseHeaders) + 1; | ||
| stringToUTF8(responseHeaders, dst, dstSizeBytes); | ||
| return Math.min(lengthBytes, dstSizeBytes); | ||
| return stringToUTF8(responseHeaders, dst, dstSizeBytes) + 1; |
There was a problem hiding this comment.
Especially since here we do include it, right?
There was a problem hiding this comment.
Same, see PR description. Judging by docs and tests, the intended behaviour was always for the Length function to just return the actual length, and it's responsibility of whoever calls fetchGetResponseHeaders to pass enough size for both length and the null terminator.
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
lgtm with a ChangeLog entry mentioning that emscripten_fetch_get_response_headers_length change (to match the documented and intended behaviour).
RReverser
force-pushed
the
use-higher-level-str-helpers
branch
from
dc9d30c to
663b7d7
Compare
…24486) I guess one functional change is in implementation of `emscripten_fetch_get_response_headers_length`. The docs say that it just returns the length, and you must use `length + 1` for buffer size passed to `emscripten_fetch_get_response_headers`, and that's what tests do, but we were actually returning `length + 1` already. This seems like a bugfix, but happy to revert if deemed too risky.
2 participants
I guess one functional change is in implementation of
emscripten_fetch_get_response_headers_length.The docs say that it just returns the length, and you must use
length + 1for buffer size passed toemscripten_fetch_get_response_headers, and that's what tests do, but we were actually returninglength + 1already. This seems like a bugfix, but happy to revert if deemed too risky.