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

BufReader Stdin or StdinLock #6755

Open
pickfire opened this issue Feb 17, 2021 · 12 comments · May be fixed by #13682
Open

BufReader Stdin or StdinLock #6755

pickfire opened this issue Feb 17, 2021 · 12 comments · May be fixed by #13682
Assignees
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-perf Lint: Belongs in the perf lint group T-middle Type: Probably requires verifiying types

Comments

@pickfire
Copy link
Contributor

pickfire commented Feb 17, 2021

What it does

Suggest using StdinLock rather than BufReader::new(io::stdin()) since it is already buffered. We might also want to take care of let stdin = io::stdin(); BufReader::new(stdin) and also BufReader::new(stdinlock) (there could be people doing this).

Categories (optional)

  • Kind: perf

Locking stdin also provide benefit for not having to re-lock it on every read.

std::io::stdin() already is buffered. Re-buffering it only increase the memcpy calls.

Source https://users.rust-lang.org/t/correct-way-for-taking-large-input-from-keyboard/55304/11
Related to #1805

For example:

  • Recommend locking of stdin to reduce re-locking
  • Suggest not re-buffering stdin lock which already implements BufRead

Drawbacks

None.

Example

let reader = BufReader::new(io::stdin());  // this could a separate variable or StdinLock

Could be written as:

let stdin = io::stdin();
let reader = stdin.lock();
@pickfire pickfire added the A-lint Area: New lints label Feb 17, 2021
@camsteffen
Copy link
Contributor

camsteffen commented Feb 17, 2021

Shouldn't BufReader::new(io::stdin()) be replaced by io::stdin().lock() (no BufReader)? StdinLock implements BufRead.

@camsteffen camsteffen added good-first-issue These issues are a good way to get started with Clippy L-perf Lint: Belongs in the perf lint group T-middle Type: Probably requires verifiying types labels Feb 17, 2021
@pickfire
Copy link
Contributor Author

Yes, I wrote it wrongly. I made the same mistake which the lint should solve.

@camsteffen
Copy link
Contributor

I think this should also include BufWriter::new(std::io::stdout()).

@pickfire
Copy link
Contributor Author

@camsteffen I don't think so as Stdout does not implements BufWrite.

@camsteffen
Copy link
Contributor

Ah you're right.

@arya-k
Copy link
Contributor

arya-k commented Apr 6, 2021

@rustbot claim

@CBenoit
Copy link
Contributor

CBenoit commented Nov 14, 2023

Hi @arya-k! Do you still plan to work on that?

@arya-k
Copy link
Contributor

arya-k commented Nov 14, 2023 via email

@CBenoit
Copy link
Contributor

CBenoit commented Nov 14, 2023

Sounds good, I’ll claim it

@rustbot claim

@rustbot rustbot assigned CBenoit and unassigned arya-k Nov 14, 2023
@yawara
Copy link
Contributor

yawara commented Nov 9, 2024

Hi @CBenoit , are you still working on this issue?

@CBenoit
Copy link
Contributor

CBenoit commented Nov 10, 2024

Hi, are you still working on this issue?

I completely forgot about it. Feel free to claim!

@yawara
Copy link
Contributor

yawara commented Nov 11, 2024

@CBenoit Thank you! I claim it.

@rustbot claim

@rustbot rustbot assigned yawara and unassigned CBenoit Nov 11, 2024
@yawara yawara linked a pull request Nov 13, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-perf Lint: Belongs in the perf lint group T-middle Type: Probably requires verifiying types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants