Conversation
anonrig
force-pushed
the
yagiz/add-validate-base64
branch
6 times, most recently
from
1c4b717 to
2e82bc5
Compare
anonrig
force-pushed
the
yagiz/add-validate-base64
branch
from
2e82bc5 to
f27ca67
Compare
|
@lemire any objections/concerns for landing this? |
|
@anonrig Happy to merge if you recommend it.
|
|
Also, please do make sure that you want to exact functionality. :-) As once we merge it in, we might be stuck supporting it as it is. :-) I am just careful about adding new functionality. |
|
There is a trivial conflict. |
I'll take a look @lemire |
|
There is no objection on my part. See my comments. I am mostly just concerned about making sure that we offer the right functionality. |
|
I can rebase this pull-request, if we're ok with it? Can you respond with a thumbs up or a thumbs down? |
|
|
||
| simdutf_warn_unused bool validate_base64(const char* input, size_t size) noexcept { | ||
| if (simdutf_unlikely(size == 0)) { | ||
| return false; |
There was a problem hiding this comment.
shouldn't this return true?
| if (padding > 2 || i < size - 2) { | ||
| return false; | ||
| } | ||
| } else if (base64_lookup[c] == 0 && c != 'A') { |
There was a problem hiding this comment.
why the 'A' check? if c is 'A' (0x41) the table gives 0? please explain.
| } | ||
| } | ||
|
|
||
| void validate_base64(std::span<const char> chardata) { |
There was a problem hiding this comment.
this file implements a differential fuzzer which does not make sense when all implementations are using the same fallback function.
I would rather see unit tests added.
| * @param len the length of the string in bytes. | ||
| * @return true if and only if the string is valid base64. | ||
| */ | ||
| simdutf_warn_unused bool validate_base64(const char *buf, size_t len) noexcept; |
There was a problem hiding this comment.
it would be good with a span overload for this function!
| return (length + 2)/3 * 4; // We use padding to make the length a multiple of 4. | ||
| } | ||
|
|
||
| // Lookup table for valid base64 characters. |
There was a problem hiding this comment.
can this be moved inside the function, to reduce scope and visibility?
|
Thanks for the review @pauldreik. I'll address them soon. |
Sure, sure. |
5 participants
Let's add a simplistic implementation where we can iterate for performance later.
Fixes #566