Skip to content

use ? instead of try! - #158

Merged
dvdplm merged 5 commits into
masterfrom
na-kill-try
Oct 1, 2019
Merged

dvdplm merged 5 commits into
masterfrom
na-kill-try

Conversation

@niklasad1

Copy link
Copy Markdown
Contributor

No description provided.

@dvdplm dvdplm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(Although I don't think try! is deprecated, is it? Not recommended for sure, but does it print a warning?)

@niklasad1 niklasad1 changed the title remove deprecated try! use ? instead of try! Oct 1, 2019
@niklasad1

niklasad1 commented Oct 1, 2019 •

Copy link
Copy Markdown
Contributor Author

(Although I don't think try! is deprecated, is it? Not recommended for sure, but does it print a warning?)

my bad, the warnings appear only on the nightly toolchain

@ordian

ordian commented Oct 1, 2019

Copy link
Copy Markdown
Member

Does it affect generated code?
FYI: rust-lang/rust#62672 (comment)

@dvdplm

dvdplm commented Oct 1, 2019

Copy link
Copy Markdown
Contributor

@niklasad1 can you rebase please? :)

@niklasad1

Copy link
Copy Markdown
Contributor Author

Does it affect generated code?

The binaries are not identical but I benched the following:

pub fn try_operator(a: Result<usize, usize>) -> Result<usize, usize> {
    Ok(a?)
}

pub fn try_macro(a: Result<usize, usize>) -> Result<usize, usize> {
    Ok(r#try!(a))
}

try was ~1 ps faster...... seems to

@ordian

ordian commented Oct 1, 2019

Copy link
Copy Markdown
Member

The binaries are not identical but I benched the following:

pub fn try_operator(a: Result<usize, usize>) -> Result<usize, usize> {
    Ok(a?)
}

pub fn try_macro(a: Result<usize, usize>) -> Result<usize, usize> {
    Ok(r#try!(a))
}

try was ~1 ps faster...... seems to

The problem was with more complex (nested) expressions and the compiler not being able to optimize the overhead away. I think it's fine in our case.

@dvdplm
dvdplm merged commit 9c86167 into master Oct 1, 2019
@ordian
ordian deleted the na-kill-try branch October 1, 2019 17:58
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.

3 participants