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

Remove extra BufReader, BufWriter, type fish #6

Closed
wants to merge 2 commits into from

Conversation

louy2
Copy link
Contributor

@louy2 louy2 commented Feb 29, 2020

The title is what I set out to do but I set up my editor so it auto-formats the source and before I realized I made all my fixes based on rustfmt formatted code 😞

I decided to at least separate out the formatting change to its own commit. If you don't like it let me know.

@koraa
Copy link
Owner

koraa commented Mar 1, 2020

Thank you for your contribution! It's great having other people look at the code :)

Now, to be honest, I am not entirely sure I understand the change actually. What purpose does removing the BufWriter serve? I did use it intentionally to limit the number of write() system calls…

Regarding the formatting changes, I am not entirely sure how I feel about that; personally I have always been someone who takes great care to format the code for readability manually, so I have often felt that there are actually places where the automated formatting is harder to read…

On the other hand, having a style that is familiar across many projects if a huge benefit to contributors!

In the actual diff I can see examples where rustfmt improves consistency and style, but I can also see examples of the opposite…maybe you could try to provide a config file to make these look better? I am thinking specifically of the increased match statement verbosity in some cases…the rest still looks fine…

@louy2
Copy link
Contributor Author

louy2 commented Mar 2, 2020

What purpose does removing the BufWriter serve? I did use it intentionally to limit the number of write() system calls…

stdin.lock() gives you a StdinLock, which implements BufRead, meaning that it is already buffered, so no need to add another buffer on top. Let me also quote the source:

pub struct StdinLock<'a> {
    inner: MutexGuard<'a, BufReader<Maybe<StdinRaw>>>,
}

maybe you could try to provide a config file to make these look better? I am thinking specifically of the increased match statement verbosity in some cases

Of course! If you can give me an example so I can have a better idea of what you mean.

@koraa
Copy link
Owner

koraa commented Mar 3, 2020

stdin.lock() gives you a StdinLock, which implements BufRead, meaning that it is already buffered, so no need to add another buffer on top. Let me also quote the source:

Bloody hell, you are right! I completely overlooked this…what a weird design choice…
Actually it seems stdin()/stdout()/stderr(), which means we have to also implement a rawStdin() for use with the zero copy reader…

Would you be willing to do add this PR too?

Of course! If you can give me an example so I can have a better idea of what you mean.

Making sure all braces in match statements are on the same line as the match should suffice. Please also open a dedicated pr for the style changes and add a rust lint config to at least signal that we have a defined code style, even if it's the default one (at the moment).

@koraa
Copy link
Owner

koraa commented Mar 4, 2020

Manually merged! Applied rustfmt to all rust files, did not use a rawStdin() implementation, because that turned out to not really improve performance.

Thanks for your contribution!

@koraa koraa closed this Mar 4, 2020
@dippi
Copy link
Contributor

dippi commented Mar 6, 2020

which means we have to also implement a rawStdin() for use with the zero copy reader…

BufReader uses directly the slice received as parameter if it's larger than the internal buffer avoiding the copy. https://doc.rust-lang.org/src/std/io/buffered.rs.html#242-248
That could explain why there wasn't a noticeable performance improvement.

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.

None yet

3 participants