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

Refactor strchr(p, 0) -> p + strlen(p) #1022

Closed
wants to merge 1 commit into from
Closed

Conversation

AreaZR
Copy link
Contributor

@AreaZR AreaZR commented Jan 10, 2024

They are both equivalent. However, given that strchr actually in fact just special cases 0, it is better to just make the intention clearer anyway by conveying in the code itself that this is what is happening.

They are both equivalent. However, given that strchr actually in fact just special cases 0, it is better to just make the intention clearer anyway by conveying in the code itself that this is what is happening.
@AreaZR AreaZR requested a review from bsdjhb as a code owner January 10, 2024 18:07
while (--t >= buf && *t == '\\')
continuation = !continuation;
for (t = buf; *t && *t == '\\'; t++)
continuation ^= 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why continuation ^= 1? This makes understanding the logic even harder. IMO the previous one is better here.

Copy link
Contributor

@igalic igalic left a comment

Choose a reason for hiding this comment

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

a lot of this feels like code churn, and it's doing way more than the commit message says

it's also not split one commit per subsystem. that's the absolute bare minimum for this PR to be considered

@bsdimp
Copy link
Member

bsdimp commented Feb 2, 2024

Since this doesn't solve an actual problem, make things demonstrably faster, etc. I'm going to close it. IT's too churny and only introduces risk of regression.

@bsdimp bsdimp closed this Feb 2, 2024
@AreaZR AreaZR deleted the strchr branch February 2, 2024 22:35
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.

4 participants