Issue #16 references not having atob and btoa (enc/dec base64)#1032
Issue #16 references not having atob and btoa (enc/dec base64)#1032graham wants to merge 2 commits intoquickjs-ng:masterfrom
Conversation
I've started with naive implementation, there is significant discussion about how to make this faster: https://stackoverflow.com/questions/342409/ If there is interest I rewrite with some of these tricks.
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
Left some comments. General style note: our code is usually pretty dense with not too many blank lines or comments. If it's clear from context what a line of code is doing, it doesn't need a comment.
| // Calculate output length (including padding) | ||
| size_t out_len = 4 * ((len + 2) / 3) + 1; | ||
| // Allocate memory for output string (plus null terminator) | ||
| char* out = (char*)js_mallocz(ctx, out_len); |
There was a problem hiding this comment.
Unnecessary cast but more substantially, allocating should preferably be done by the caller, because then the caller can use e.g. a stack-allocated buffer for small inputs.
| size_t i, j; | ||
|
|
||
| for (i = 0, j = 0; i < len; i += 3, j += 4) { | ||
| uint32_t triple = (bin[i] << 16); |
There was a problem hiding this comment.
| uint32_t triple = (bin[i] << 16); | |
| uint32_t triple = (uint8_t)bin[i] << 16; |
Otherwise if char is signed and bin[i] == -42 (for example), you get unexpected output; plus, shifting into the sign bit is ill-defined behavior in C. If you were to shift by 24 bits, you'd have to write it as (uint32_t)(uint8_t)bin[i] << 16 (or change bin to uint8_t *)
| JSValue v; | ||
|
|
||
| if (argc > 0) { | ||
| fmt_str = JS_ToCString(ctx, argv[0]); |
There was a problem hiding this comment.
| fmt_str = JS_ToCString(ctx, argv[0]); | |
| fmt_str = JS_ToCString(ctx, argv[0]); | |
| if (!fmt_str) | |
| return JS_EXCEPTION; |
|
Could you please adjust this to comply with the standard? At first glance it's missing at least the "remove ASCII whitespace" step. Also, atob/btoa should at least be static to avoid namespace pollution. Or a public API, but then not with these names. |
|
Thank you for all the feedback and notes. I'll try to clean this up and make sure I implement the spec correctly. If @bptato ends up finding their solution before I finish at least I can say I learned quite a bit :) |
3 participants
I've started with naive implementation, there is significant discussion about how to make this faster:
https://stackoverflow.com/questions/342409/
If there is interest I rewrite with some of these tricks.