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

fix compiler error in 'std.fmt.Parser.until' #22129

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

allocgator
Copy link
Contributor

Fixes _ = &std.fmt.Parser.until; in #20505

Copy link
Member

@Vexu Vexu left a comment

Choose a reason for hiding this comment

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

until should not be using ++, a better solution would be to call nextCodepoint like the other functions next to it and return a slice like it did before #18533

@allocgator
Copy link
Contributor Author

until should not be using ++, a better solution would be to call nextCodepoint like the other functions next to it and return a slice like it did before #18533

Would it involve converting u21 to u8 ?

@Vexu
Copy link
Member

Vexu commented Dec 9, 2024

No, it would look something like:

pub fn until(self: *@This(), ch: u21) []const u8 {
    const start = self.iter.i;
    while (self.peek(0)) |code_point| {
        if (code_point == ch)
            break;
        _ = self.iter.nextCodepoint();
    }
    return self.iter.bytes[start..self.iter.i];
}

@allocgator allocgator requested a review from Vexu December 10, 2024 02:50
@Vexu Vexu enabled auto-merge (squash) December 10, 2024 20:25
@andrewrk andrewrk disabled auto-merge December 10, 2024 21:12
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

This kind of easily tested change should come with unit test coverage.

@Vexu
Copy link
Member

Vexu commented Dec 10, 2024

Fair enough, none of the previous fixes in #20505 added any specific tests so I figured the existing usage as part of format strings would be sufficient.

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.

3 participants