FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

add simdutf::validate_base64 by anonrig · Pull Request #568 · simdutf/simdutf · GitHub

/ simdutf Public

add simdutf::validate_base64 - #568

Open
anonrig wants to merge 3 commits into
masterfrom
yagiz/add-validate-base64
ConversationCommits (3)ChecksFiles changed
Open

add simdutf::validate_base64#568
anonrig wants to merge 3 commits into
masterfrom
yagiz/add-validate-base64

Conversation

anonrig commented Sep 27, 2024

Copy link
Copy Markdown
Member

Let's add a simplistic implementation where we can iterate for performance later.

Fixes #566

anonrig requested a review from lemire September 27, 2024 16:10
anonrig force-pushed the yagiz/add-validate-base64 branch 6 times, most recently from 1c4b717 to 2e82bc5 Compare September 27, 2024 16:23
anonrig force-pushed the yagiz/add-validate-base64 branch from 2e82bc5 to f27ca67 Compare September 27, 2024 16:27
anonrig commented Sep 27, 2024
View reviewed changes
Comment thread include/simdutf/implementation.h Outdated

anonrig commented Sep 29, 2024

Copy link
Copy Markdown
Member Author

@lemire any objections/concerns for landing this?

lemire commented Sep 30, 2024

Copy link
Copy Markdown
Member

@anonrig Happy to merge if you recommend it.

  • Do we want to pass a parameter to enable (for example) base64url?
  • Should we update the README to include a reference to this function?

lemire commented Sep 30, 2024

Copy link
Copy Markdown
Member

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.

lemire commented Oct 1, 2024

Copy link
Copy Markdown
Member

@anonrig

There is a trivial conflict.

anonrig commented Oct 1, 2024

Copy link
Copy Markdown
Member Author

There is a trivial conflict.

I'll take a look @lemire

WojciechMula commented Feb 19, 2025

Copy link
Copy Markdown
Collaborator

@anonrig @lemire is there anything that prevents us from merging? (apart from conflicts)

lemire commented Feb 19, 2025

Copy link
Copy Markdown
Member

@WojciechMula

There is no objection on my part. See my comments. I am mostly just concerned about making sure that we offer the right functionality.

anonrig commented Apr 26, 2025

Copy link
Copy Markdown
Member Author

I can rebase this pull-request, if we're ok with it? Can you respond with a thumbs up or a thumbs down?

pauldreik requested changes Apr 26, 2025
View reviewed changes
Comment thread src/scalar/base64.h

simdutf_warn_unused bool validate_base64(const char* input, size_t size) noexcept {
if (simdutf_unlikely(size == 0)) {
return false;

pauldreik Apr 26, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

shouldn't this return true?

Comment thread src/scalar/base64.h
if (padding > 2 || i < size - 2) {
return false;
}
} else if (base64_lookup[c] == 0 && c != 'A') {

pauldreik Apr 26, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

why the 'A' check? if c is 'A' (0x41) the table gives 0? please explain.

Comment thread fuzz/misc.cpp
}
}

void validate_base64(std::span<const char> chardata) {

pauldreik Apr 26, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread include/simdutf/implementation.h
* @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;

pauldreik Apr 26, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

it would be good with a span overload for this function!

Comment thread src/scalar/base64.h
return (length + 2)/3 * 4; // We use padding to make the length a multiple of 4.
}

// Lookup table for valid base64 characters.

pauldreik Apr 26, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

can this be moved inside the function, to reduce scope and visibility?

anonrig commented Apr 26, 2025

Copy link
Copy Markdown
Member Author

Thanks for the review @pauldreik. I'll address them soon.

lemire commented Apr 28, 2025

Copy link
Copy Markdown
Member

I can rebase this pull-request, if we're ok with it? Can you respond with a thumbs up or a thumbs down?

Sure, sure.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

pauldreik pauldreik requested changes
jasnell jasnell approved these changes
lemire Awaiting requested review from lemire

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Adding is_base64(const char*, size_t) API

5 participants

Back | FazBrowse Home | New Git URL