Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ZeroAsciiIgnoreCaseTrieCursor #4563

Merged
merged 5 commits into from
Feb 5, 2024
Merged

Conversation

sffc
Copy link
Member

@sffc sffc commented Jan 31, 2024

Last piece of the ZeroTrie improvements required to continue work on #4548 / #4031

I filed #4561 for additional follow-up work to try to reduce code duplication. I would like to not block this PR on code duplication concerns.

@sffc sffc requested a review from a team as a code owner January 31, 2024 02:39
c: u8,
) -> Option<u8> {
// BinarySpans is tricky to implement because the state can no longer be simply a trie
debug_assert!(
Copy link
Member

Choose a reason for hiding this comment

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

Should this return None here without assertions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without assertions it returns None on line 468, if it actually hits a span node.

Copy link
Member

Choose a reason for hiding this comment

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

There's already a debug assert there, why this as well? There should at least be a comment linking to the line and what the gigo value is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I expanded these comments

/// For examples, see [`ZeroAsciiIgnoreCaseTrie::cursor()`].
// Clone but not Copy: <https://stackoverflow.com/q/32324251/1407170>
#[derive(Debug, Clone)]
pub struct ZeroAsciiIgnoreCaseTrieCursor<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

not a big fan of adding yet another type, but you already know that

Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote some ideas on how to resolve this in #4561

@sffc sffc merged commit 6fda1e8 into unicode-org:main Feb 5, 2024
30 checks passed
@sffc sffc deleted the zt-case-cursor branch February 5, 2024 23:18
@sffc sffc removed the request for review from Manishearth February 5, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants