[wasm2js] Fix base64 encoding #1670
Conversation
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Good catch!
How about using uint8_t instead of unsigned? Also might as well change base64Encode to receive a vector of those.
Please also add a test.
yurydelendik
force-pushed
the
fix-base64-js
branch
from
4b65dd3 to
89700fc
Compare
Fixed
This change goes all the way to Module -- maybe different patch.
Done |
yurydelendik
changed the title
src/wasm2js.h
Outdated
| (((int) data[i + 0]) << 16) | | ||
| (((int) data[i + 1]) << 8) | | ||
| (((int) data[i + 2]) << 0); | ||
| (((uint8_t) data[i + 0]) << 16) | |
There was a problem hiding this comment.
Hmm, wait, this doesn't seem right - as uint8_t it would just be 0 after doing << 16, wouldn't it? I think we want uint32_t here.
Please also make sure we have a test that verifies that.
There was a problem hiding this comment.
the attached test verifies just that.
There was a problem hiding this comment.
the (uint8_t) cast is needed to fix the bug. the "the integer promotions" extends it to integer. I can be explicit and add (uint32_t) everywhere.
There was a problem hiding this comment.
Great, thanks, I think it's less surprising to read that way. lgtm.
2 participants
The
static std::string base64Encode(std::vector<char> &data) {uses signed char in input data. The((int)data[0])converts it the signed int, making '\xFF' char into -1. The patch fixes casting.