Conversation
tcharding
left a comment
tcharding
left a comment
There was a problem hiding this comment.
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.
| // machinery for a custom type. | ||
| pub fn to_str_radix(&self, base: u8) -> String { | ||
| unsafe { | ||
| assert!(base >= 2 && base <= 36, "invalid base"); |
There was a problem hiding this comment.
Why limit to 36 please (as opposed to 64)? Perhaps a function that checks valid bases that has comments would be useful?
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
This is a nice check, perhaps an error return instead of panic would be nice on the users?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
| /// 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. |
There was a problem hiding this comment.
GitHub fail, I can't see all of these lines.
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 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. |
3 participants
Mpz#[repr(transparent)]#[inline]attributes