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

Try to replace .expects with safe internal APIs #31

Open
AaronKutch opened this issue Oct 7, 2018 · 2 comments
Open

Try to replace .expects with safe internal APIs #31

AaronKutch opened this issue Oct 7, 2018 · 2 comments

Comments

@AaronKutch
Copy link
Contributor

There are several places in the library with .expects that could be eliminated. However, this is low on our list of priorities and I am just filing this issue to remind me in the future.

@Robbepop
Copy link
Owner

Could you please provide further information and an example within the code so that we do not forget the track of this in the future?

@AaronKutch
Copy link
Contributor Author

AaronKutch commented Oct 11, 2018

When searching for .expect in apint, it turns up 29 results as of commit 504.
e.x.

    /// Sets the sign bit of this `ApInt` to one (`1`).
    pub fn set_sign_bit(&mut self) {
        let sign_bit_pos = self.width().sign_bit_pos();
        self.set_bit_at(sign_bit_pos)
            .expect("`BitWidth::sign_bit_pos` always returns a valid `BitPos`
                     for usage in the associated `ApInt` for operating on bits.")
    }

I think that the only good use of .expect today is in more informative error messages. unreachable has a much smaller footprint in code and when it panics it returns its location in code, the only two things we really need.
I believe that all of these could be removed by doing one of these on a case by case basis:

  • The internal error handling should be moved elsewhere by better internal APIs if easy and it eliminates multiple expects.
  • It should be replaced by unreachable! and the explanation put in a comment by the unreachable!. We should also make a note in the future invariants documentation section to search for unreachable in the code to find the places where invariant incorrectness could occur.
  • If we find one that should and can be returned to the user, refactor to have the function return a Result.

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

No branches or pull requests

2 participants