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

chore(docs): change single character pattern #199

Closed

Conversation

joshuachp
Copy link

Change the single character string to a char in the examples and README to fix the "clippy::single-char-pattern" warning.

Change the single character string to a char in the examples and README
to fix the "clippy::single-char-pattern" warning.
@BurntSushi
Copy link
Owner

I don't really agree with this lint. I think it has dubious upsides and potential perf downsides, although I'd be hard-pressed to write a real world benchmark where it matters.

@BurntSushi BurntSushi closed this Jun 15, 2024
@joshuachp
Copy link
Author

joshuachp commented Jun 15, 2024

I was curious so I checked this out on godbolt.org and I'm pretty confident they are the same. The only difference is that using a string generates the clippy warning, but in the end it's a minor thing anyway 😄.

FYI: in the closure the only change I could find is a lea for the string, while there is a move for the char. While with opt-level = 2 the asm is the same as the char.

https://godbolt.org/z/68Mf9eKnW

@BurntSushi
Copy link
Owner

Yes, I was careful in my wording.

There are a lot of Clippy lints I don't really agree with. And I generally don't like PRs that just exist to fix those lints. Basically, I don't judge revisions to code based on whether they satisfy a lint tool. I judge them based on whether they improve the code. And in the grand scheme of things, I don't think find('a') versus find("a") embodies a distinction with a meaningful difference. You can use either and it will be fine.

@BurntSushi
Copy link
Owner

To be clear, I would like to eventually use Clippy in my projects. I do like lint tools. But I just haven't had the time or inclination to set it up. Because "set it up" means going through the lints and picking which ones I like and don't like. It's a lot of work.

@joshuachp
Copy link
Author

joshuachp commented Jun 15, 2024

Completely agreed, sorry if it seemed I was pushing this. You nerd sniped me with your comment and had to check this out 😆

I opened the PR just because in a new project I copy pasted the example and got the warning, so I though other people probably would encounter it too. Anyway, thank you for your time.

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