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

Fixes used by VDF #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixes used by VDF #32

wants to merge 1 commit into from

Conversation

DemiMarie
Copy link

  • Make Mpz #[repr(transparent)]
  • Add #[inline] attributes
  • Delete trailing whitespace

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.

It wasn't exactly clear to me why these changes are being done (but I don't know what VDF is), for example what's the benefit here of inlining (assuming its an optimization is there a reason for it?)

Explanations and reasoning aside the PR looks good to me.

@@ -158,31 +165,22 @@ impl Mpz {
// machinery for a custom type.
pub fn to_str_radix(&self, base: u8) -> String {
unsafe {
assert!(base >= 2 && base <= 36, "invalid base");

Choose a reason for hiding this comment

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

Why limit to 36 please (as opposed to 64)? Perhaps a function that checks valid bases that has comments would be useful?

Copy link
Author

Choose a reason for hiding this comment

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

I don’t remember why I chose that limit when I wrote the code. The correct limit is whatever the C API specifies.

let len = __gmpz_sizeinbase(&self.mpz, base as c_int) as usize + 2;
let len = {
let len = __gmpz_sizeinbase(&self.mpz, base as c_int) as usize;
assert!(usize::MAX - len >= 2, "capacity overflow");

Choose a reason for hiding this comment

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

This is a nice check, perhaps an error return instead of panic would be nice on the users?

Copy link
Author

Choose a reason for hiding this comment

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

This should never happen, so a panic is a better choice.

Ok(s) => s,
Err(_) => panic!("GMP returned invalid UTF-8!")
}
let string_len = strnlen(vector.as_ptr(), len);

Choose a reason for hiding this comment

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

Excuse my ignorance; is the memory pointed to by Rust vectors guaranteed to be null terminated? I did a quick search but couldn't find docs on this, thanks in advance for the lesson :)

Copy link
Author

Choose a reason for hiding this comment

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

It is not, but my understanding is that __gmpz_get_str NUL-terminates the buffer it writes to. I could be wrong, though; I wrote this code around two years ago.

Comment on lines -261 to +265
/// This function performs some trial divisions, then reps Miller-Rabin probabilistic primality tests. A higher reps value will reduce the chances of a non-prime being identified as “probably prime”. A composite number will be identified as a prime with a probability of less than 4^(-reps). Reasonable values of reps are between 15 and 50.
/// This function performs some trial divisions, then reps Miller-Rabin probabilistic primality tests. A higher reps value will reduce the chances of a non-prime being identified as “probably prime”. A composite number will be identified as a prime with a probability of less than 4^(-reps). Reasonable values of reps are between 15 and 50.

Choose a reason for hiding this comment

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

GitHub fail, I can't see all of these lines.

@cbarrick
Copy link

for example what's the benefit here of inlining

It allows cross-crate inlining without LTO.

IIUC, by default, an rlib will contain an optimized representation of the source code for generic functions but will only contain the compiled form of non-generic functions. The source representation is required for cross-crate inlining.

The #[inline] attribute tells the compiler to include the source representation in the rlib, and hints to LLVM that it should inline these function calls.

Since all of the annotated functions are small, inlining seems reasonable, especially cross crate.

Apologies for the drive-by comment.

@tcharding
Copy link

for example what's the benefit here of inlining

It allows cross-crate inlining without LTO.

IIUC, by default, an rlib will contain an optimized representation of the source code for generic functions but will only contain the compiled form of non-generic functions. The source representation is required for cross-crate inlining.

The #[inline] attribute tells the compiler to include the source representation in the rlib, and hints to LLVM that it should inline these function calls.

Since all of the annotated functions are small, inlining seems reasonable, especially cross crate.

Apologies for the drive-by comment.

Mad, thanks for the info man.

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