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

use top level fs functions where appropriate #56258

Merged
merged 1 commit into from
Dec 8, 2018

Conversation

euclio
Copy link
Contributor

@euclio euclio commented Nov 26, 2018

This commit replaces many usages of File::open and reading or writing
with fs::read_to_string, fs::read and fs::write. This reduces code
complexity, and will improve performance for most reads, since the
functions allocate the buffer to be the size of the file.

I believe that this commit will not impact behavior in any way, so some
matches will check the error kind in case the file was not valid UTF-8.
Some of these cases may not actually care about the error.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 26, 2018
let stamp_contents = match fs::read(stamp) {
Ok(contents) => contents,
Err(ref err) if err.kind() == ErrorKind::InvalidData => {
panic!("could not read contents of stamp");
Copy link
Member

Choose a reason for hiding this comment

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

Why would InvalidData come up? I could maybe see it if we were reading to string, but with just a Vec return value I don't quite see it -- and this does appear to be new code

Copy link
Contributor Author

@euclio euclio Nov 27, 2018

Choose a reason for hiding this comment

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

I was trying to keep the behavior that errors opening the file are ignored but errors reading the file panic. You're right that InvalidData won't come up when we're reading bytes -- it's probably best to just do something like let contents = fs::read(stamp).ok();?

EDIT: Pushed a commit fixing this.


let mut f = try_err!(File::open(&entry), &entry);
try_err!(f.read_to_end(&mut content), &entry);
let content = try_err!(fs::read(&entry), &entry);
Copy link
Member

Choose a reason for hiding this comment

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

Technically I think this might be a change in performance so cc @rust-lang/rustdoc but I think the new behavior is better, not worse, since we'll allocate correctly versus just guessing.

src/libsyntax/source_map.rs Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

If you could squash the commits here into one that'd be great; r=me modulo that

@euclio
Copy link
Contributor Author

euclio commented Nov 27, 2018

Squashed.

@nikomatsakis
Copy link
Contributor

@bors r=Mark-Simulacrum

r? @Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Nov 29, 2018

📌 Commit be89dff has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 29, 2018
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 29, 2018
…crum

use top level `fs` functions where appropriate

This commit replaces many usages of `File::open` and reading or writing
with `fs::read_to_string`, `fs::read` and `fs::write`. This reduces code
complexity, and will improve performance for most reads, since the
functions allocate the buffer to be the size of the file.

I believe that this commit will not impact behavior in any way, so some
matches will check the error kind in case the file was not valid UTF-8.
Some of these cases may not actually care about the error.
@bors
Copy link
Contributor

bors commented Nov 30, 2018

☔ The latest upstream changes (presumably #56381) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 30, 2018
@euclio
Copy link
Contributor Author

euclio commented Dec 1, 2018

Rebased.

@euclio
Copy link
Contributor Author

euclio commented Dec 5, 2018

Ping @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 5, 2018

📌 Commit d809d21 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 5, 2018
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 5, 2018
…crum

use top level `fs` functions where appropriate

This commit replaces many usages of `File::open` and reading or writing
with `fs::read_to_string`, `fs::read` and `fs::write`. This reduces code
complexity, and will improve performance for most reads, since the
functions allocate the buffer to be the size of the file.

I believe that this commit will not impact behavior in any way, so some
matches will check the error kind in case the file was not valid UTF-8.
Some of these cases may not actually care about the error.
@bors
Copy link
Contributor

bors commented Dec 6, 2018

☔ The latest upstream changes (presumably #54517) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 6, 2018
@euclio
Copy link
Contributor Author

euclio commented Dec 6, 2018

Rebased again. Notably in the merge I tweaked the logic a bit to avoid a clone if the file doesn't contain UTF-8. https://github.com/rust-lang/rust/blob/4ed9da8816db8e3273a51061482bd05ba6f43509/src/libsyntax/ext/source_util.rs#L171-L177

@bors
Copy link
Contributor

bors commented Dec 7, 2018

☔ The latest upstream changes (presumably #56566) made this pull request unmergeable. Please resolve the merge conflicts.

This commit replaces many usages of `File::open` and reading or writing
with `fs::read_to_string`, `fs::read` and `fs::write`. This reduces code
complexity, and will improve performance for most reads, since the
functions allocate the buffer to be the size of the file.

I believe that this commit will not impact behavior in any way, so some
matches will check the error kind in case the file was not valid UTF-8.
Some of these cases may not actually care about the error.
@Mark-Simulacrum
Copy link
Member

@bors delegate+

You should be able to re-approve from here once you've rebased.

@bors
Copy link
Contributor

bors commented Dec 7, 2018

✌️ @euclio can now approve this pull request

@euclio
Copy link
Contributor Author

euclio commented Dec 7, 2018

Travis passed.

@bors r+

@bors
Copy link
Contributor

bors commented Dec 7, 2018

📌 Commit 2f62265 has been approved by euclio

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 7, 2018
@bors
Copy link
Contributor

bors commented Dec 7, 2018

⌛ Testing commit 2f62265 with merge 0a77980...

bors added a commit that referenced this pull request Dec 7, 2018
use top level `fs` functions where appropriate

This commit replaces many usages of `File::open` and reading or writing
with `fs::read_to_string`, `fs::read` and `fs::write`. This reduces code
complexity, and will improve performance for most reads, since the
functions allocate the buffer to be the size of the file.

I believe that this commit will not impact behavior in any way, so some
matches will check the error kind in case the file was not valid UTF-8.
Some of these cases may not actually care about the error.
@bors
Copy link
Contributor

bors commented Dec 8, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: euclio
Pushing 0a77980 to master...

@bors bors merged commit 2f62265 into rust-lang:master Dec 8, 2018
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 11, 2018
@euclio euclio deleted the fs-read-write branch December 11, 2018 05:46
bors added a commit that referenced this pull request Dec 11, 2018
Fix build of the `build-manifest` tool

Accidentally broken in #56258!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants