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

Update code to use newer rust versions and eliminate use of obsolete methods #33

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mikelodder7
Copy link

No description provided.

Signed-off-by: Michael Lodder <[email protected]>
Signed-off-by: Michael Lodder <[email protected]>
@andersk
Copy link

andersk commented Jul 9, 2020

MaybeUninit::uninit().assume_init() has exactly the same undefined behavior problem for which uninitialized() was deprecated. The point of MaybeUninit is that you shouldn’t call .assume_init() until after it’s actually been initialized.

@mikelodder7
Copy link
Author

This is mostly a straight across fix to bring to 2018 edition. If you want, we can update it to something else but this shows that its better than uninitialized()

@andersk
Copy link

andersk commented Jul 10, 2020

I’m not sure where you get from that page that it’s better than uninitialized()—the page specifically says:

Note that there are other ways to trigger this UB without explicitely using mem::uninitialized::<T>(), such as

MaybeUninit::<T>::uninit() // Uninitialized `MaybeUninit<T>` => Fine
        .assume_init()         // Back to an uninitialized `T` => UB

(UB = undefined behavior), which is calling out the latter pattern as equally bad, not better.

Copy link

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

Hi, I think I just re-did a bunch of what you do in this PR here, perhaps we can merge/rebase and pick the best changes.

Sorry I didn't review this before doing mine :)

I left some comments inline, my main complaint is that this PR could have been done in a bunch of separate commits, there were a few different things going on in each one IIUC.

@@ -81,7 +82,7 @@ impl Mpf {

pub fn new(precision: usize) -> Mpf {
unsafe {
let mut mpf = uninitialized();
let mut mpf = MaybeUninit::uninit().assume_init();

Choose a reason for hiding this comment

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

I think the pattern we want is

        let mut mpf = MaybeUninit::uninit();
        unsafe {
            __gmpf_init2(mpf.as_mut_ptr(), precision as c_ulong);
            let mpf = mpf.assume_init();
            Mpf { mpf }
        }

use super::sign::Sign;
use ffi::*;
use libc::{c_char, c_double, c_int, c_ulong};
use crate::mpz::{mpz_struct, Mpz, mpz_ptr, mpz_srcptr};

Choose a reason for hiding this comment

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

I like all these changes :)

@@ -206,7 +206,7 @@ impl Error for ParseMpqError {
"invalid rational number"
}

fn cause(&self) -> Option<&'static Error> {
fn cause(&self) -> Option<&'static dyn Error> {

Choose a reason for hiding this comment

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

yep, seems you were using Clippy to steer this too :)

@@ -256,6 +259,19 @@ impl Mpz {
}
}

pub fn div_rem(&self, other: &Mpz) -> (Mpz, Mpz) {

Choose a reason for hiding this comment

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

I can't work out why you add this? Was there a clippy warning after changing to 2018 that I didn't see?

Copy link
Author

Choose a reason for hiding this comment

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

I use it for certain arithmetic operations where I get both results with one call

Mpz::from_str_radix("1010010011", 2).unwrap().popcount() == 5;
assert_eq!(Mpz::from_str_radix("1010010011", 2).unwrap().popcount(), 5);

Choose a reason for hiding this comment

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

Oh nice, I did this one wrong. I fixed my change in line with this :)

@tcharding tcharding mentioned this pull request Feb 12, 2021
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