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

Avoid unwrapping in libterm #29992

Closed
frewsxcv opened this issue Nov 23, 2015 · 3 comments
Closed

Avoid unwrapping in libterm #29992

frewsxcv opened this issue Nov 23, 2015 · 3 comments
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@frewsxcv
Copy link
Member

Instead of this:

                        if !stack.is_empty() {
                            match stack.pop().unwrap() {
                                ...
                            }
                        } else {
                            ...
                        }

do:

                        match stack.pop() {
                            Some(..) => ...
                            None => ...
                        }

Similarly, instead of this:

                        if stack.len() > 1 {
                            match (stack.pop().unwrap(), stack.pop().unwrap()) {
                                (Number(y), Number(x)) => ...
                                _ => ...
                            }
                        } else {
                            ...
                        }

do:

                        match (stack.pop(), stack.pop()) {
                            (Some(Number(y)), Some(Number(x))) => ...
                            (Some(_), Some(_) => ...
                            _ => ...
                        }

There are multiple instances of these patterns in this file so make sure to clean up all of them

@nrc nrc added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Nov 23, 2015
@SingingTree
Copy link
Contributor

Gonna have a go at this one unless anyone has already grabbed it.

@Stebalien
Copy link
Contributor

Or you could just re-import the current term crate (where I fixed this about a week ago)...

SingingTree added a commit to SingingTree/rust that referenced this issue Dec 2, 2015
This brings across changes made to the term library to libterm. This
includes removing instances or unwrap, fixing format string handling, and
removing a TODO.

This fix does not bring all changes across, as term now relies on cargo
deps that cannot be brought into the rust build at this stage, but has
attempted as best to cross port changes not relying on this. This notably
limits extra functionality since implemented int he Terminal trait in
Term.

This is in partly in response to rust issue rust-lang#29992.
SingingTree added a commit to SingingTree/rust that referenced this issue Dec 3, 2015
This brings across changes made to the term library to libterm. This
includes removing instances or unwrap, fixing format string handling, and
removing a TODO.

This fix does not bring all changes across, as term now relies on cargo
deps that cannot be brought into the rust build at this stage, but has
attempted as best to cross port changes not relying on this. This notably
limits extra functionality since implemented int he Terminal trait in
Term.

This is in partly in response to rust issue rust-lang#29992.
bors added a commit that referenced this issue Dec 3, 2015
This removes a number of instances of unwrap and replaces them with
pattern matching.

This is in response to rust issue #29992.
@untitaker
Copy link
Contributor

This may be closed, I don't see any unnecessary unwraps in libterm anymore.

@frewsxcv frewsxcv closed this as completed Dec 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

No branches or pull requests

5 participants