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

convert 99.9% of try!s to ?s #32390

Merged
merged 8 commits into from
Mar 23, 2016
Merged

convert 99.9% of try!s to ?s #32390

merged 8 commits into from
Mar 23, 2016

Conversation

japaric
Copy link
Member

@japaric japaric commented Mar 21, 2016

The first commit is an automated conversion using the untry tool and the following command:

$ find -name '*.rs' -type f | xargs untry

at the root of the Rust repo.

cc @rust-lang/lang @alexcrichton @brson

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@japaric
Copy link
Member Author

japaric commented Mar 21, 2016

This is, approximately, all the try!s that were left behind in the repository.

$ grep 'try!' **/*.rs | grep -v 'panictry!' | grep -v 'option_try!' | grep -v ' *//'
src/libcore/num/bignum.rs:                try!(write!(f, "{:#x}", self.base[sz-1]));
src/libcore/num/bignum.rs:                    try!(write!(f, "_{:01$x}", v, digitlen));
src/librustc_const_eval/int.rs:                match try!(self.infer(rhs)) {
src/librustc_const_eval/int.rs:                match try!(self.infer(rhs)) {
src/librustc/diagnostics.rs:Another situation in which this occurs is when you attempt to use the `try!`
src/librustc/diagnostics.rs:    let mut f = try!(File::create("foo.txt"));
src/librustc/diagnostics.rs:`try!` returns a `Result<T, E>`, and so the function must. But `main()` has
src/libserialize/json.rs:            try!(write!($enc.writer, "\"{}\"", $e));
src/libserialize/json.rs:            try!(write!($enc.writer, "{}", $e));
src/libserialize/serialize.rs:                    let ret = ($(try!(d.read_tuple_arg({ i+=1; i-1 },
src/libserialize/serialize.rs:                    $(try!(s.emit_tuple_arg({ i+=1; i-1 }, |s| $name.encode(s)));)*
src/libstd/sys/common/backtrace.rs:                                try!(writer.write_all($demangled));
src/libstd/sys/common/backtrace.rs:                                try!(writer.write_all(rest.as_bytes()));

The tool can't/won't convert try!s that are inside macros like macro_rules! or inside comments.

base,
&file_path,
&relative_file_path,
tests));
tests)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

The automatic conversion breaks alignment in multi-line statements such as this. Should this be addressed in other PRs?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be in this one directly. Otherwise we'll certainly have multiple PRs to fix local broken alignments.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, can untry be extended to warn about cases like this, to ease the process of detecting them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, can untry be extended to warn about cases like this, to ease the process of detecting them?

Yeah, it shouldn't be that hard. (famous last words)

The automatic conversion breaks alignment in multi-line statements such as this. Should this be addressed in other PRs?

Once we get a good way to detect all these cases (with the warn feature @pnkfelix mentioned above) it should easy to fix all these in a single PR; it doesn't have to be done in this one though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Abandoning the style, which causes such problems is not an option?

@pnkfelix
Copy link
Member

rubber stamping... (leaving the fixes to indentation to later PR's)

@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 22, 2016

📌 Commit 1316ad4 has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Mar 22, 2016

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

@japaric
Copy link
Member Author

japaric commented Mar 22, 2016

untry now warns when converting multi-line try!s. There are 313 instances of multi-line try!s in this PR. I'm going to rebase this and fix the alignments.

@japaric
Copy link
Member Author

japaric commented Mar 22, 2016

@bors: r=pnkfelix

@bors
Copy link
Contributor

bors commented Mar 22, 2016

📌 Commit 10ed4f6 has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Mar 23, 2016

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

@chris-morgan
Copy link
Member

@japaric, FYI:

grep 'try!' **/*.rs | grep -v 'panictry!' | grep -v 'option_try!' | grep -v ' *//'

grep '\btry!' **/*.rs | grep -v //

Or with ag and a slightly improved comment filter,

ag --rust '\btry!' | grep -v '//.*try!'

@japaric
Copy link
Member Author

japaric commented Mar 23, 2016

Rebased and pushed a commit that enables the question_mark feature gate in the recently added rustc_borrowck crate.

@bors: r=pnkfelix

@bors
Copy link
Contributor

bors commented Mar 23, 2016

📌 Commit c548eda has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Mar 23, 2016

⌛ Testing commit c548eda with merge e2e5795...

@bors
Copy link
Contributor

bors commented Mar 23, 2016

💔 Test failed - auto-linux-64-x-android-t

@japaric
Copy link
Member Author

japaric commented Mar 23, 2016

I deleted an & by mistake while fixing the alignments 😅.

@bors: r=pnkfelix

@bors
Copy link
Contributor

bors commented Mar 23, 2016

📌 Commit c063c51 has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Mar 23, 2016

⌛ Testing commit c063c51 with merge b76f818...

bors added a commit that referenced this pull request Mar 23, 2016
convert 99.9% of `try!`s to `?`s

The first commit is an automated conversion using the [untry] tool and the following command:

```
$ find -name '*.rs' -type f | xargs untry
```

at the root of the Rust repo.

[untry]: https://github.com/japaric/untry

cc @rust-lang/lang @alexcrichton @brson
si.hStdInput = stdin.raw();
si.hStdOutput = stdout.raw();
si.hStdError = stderr.raw();

try!(unsafe {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now I'm not sure if ? was actually an improvement in cases like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

? should probably just go inside the unsafe block.

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.

10 participants