Remove &x convention in ccall in gmp
#23289
Conversation
|
Will the |
|
yes for the ccall syntax |
|
I remember someone telling me of performance issues between |
|
Yes performance has been resolved thanks to |
base/gmp.jl
Outdated
| @@ -113,56 +113,54 @@ module MPZ | |||
| # and `add!(x, a) = add!(x, x, a)`. | |||
| using Base.GMP: BigInt, Limb | |||
|
|
|||
| const mpz_t = Ref{BigInt} | |||
There was a problem hiding this comment.
Why do you remove mpz_t? I thought it helps reading tedious libgmp signatures, and comparing them to the GMP docs (which use mpz_t).
There was a problem hiding this comment.
it's also good for conciseness in this file
base/gmp.jl
Outdated
| $op(a) = $op!(BigInt(), a) | ||
| end | ||
| end | ||
|
|
||
| popcount(a::BigInt) = ccall((:__gmpz_popcount, :libgmp), Culong, (mpz_t,), &a) % Int | ||
| popcount(a::BigInt) = ccall((:__gmpz_popcount, :libgmp), Culong, (Mpz_t,), a) % Int |
There was a problem hiding this comment.
Unrelated but % Int seems dangerous here because of the Culong . Would be better to wrap in Int, even if a bit slower (which shouldn't be a concern because BigInt's are not used for performance). I can open a different PR for this.
|
Can anyone restart travis/circleci ? |
base/gmp.jl
Outdated
| @@ -113,57 +115,56 @@ module MPZ | |||
| # and `add!(x, a) = add!(x, x, a)`. | |||
| using Base.GMP: BigInt, Limb | |||
|
|
|||
| const mpz_t = Ptr{BigInt} | |||
| const bitcnt_t = Culong | |||
| const Mpz_t = Ref{BigInt} | |||
There was a problem hiding this comment.
I find Mpz_t a bit ugly as these are initials. I think this is an unrelated change, so you could do that in another PR, but if you really insist to change it now, I think I would prefer MPZ_t. Also I had introduced bitcnt_t to make it easier to match the signatures in this MPZ module with the GMP docs (as there had been some mistakes previously with using raw types like Culong), so I don't understand why you remove it now without giving motivation in an unrelated PR.
There was a problem hiding this comment.
Sure will change. I initially found it confusing since it was not capitalized, same with the other alias.
There was a problem hiding this comment.
No problem; I just picked the names from the GMP docs, but for other things I had been requested to use Julia naming style, like MPZ instead of mpz for the module name.
You'd need to rebase for circle to work, this branch doesn't contain a circle.yml. |
base/gmp.jl
Outdated
| "does not correspond to the compile time version (version $GMP_VERSION with __gmp_bits_per_limb == $GMP_BITS_PER_LIMB).\n", | ||
| msg(string("The dynamically loaded GMP library (version $(gmp_version()), | ||
| with __gmp_bits_per_limb == $(gmp_bits_per_limb()))\n", | ||
| "does not correspond to the compile time version", |
There was a problem hiding this comment.
this will print as version(version , better to keep the space between
There was a problem hiding this comment.
doesn't look like it was? either this line should have a space before the close quote or the next line should have a space before after the open quote
There was a problem hiding this comment.
Thanks, I missed this line, which also requires a space as well (I updated the other one that required a space)
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
|
part of #6080 (ref) |
|
Merge ? |
| limbs_write!(x::BigInt, a) = ccall((:__gmpz_limbs_write, :libgmp), Ptr{Limb}, (mpz_t, Clong), x, a) | ||
| limbs_finish!(x::BigInt, a) = ccall((:__gmpz_limbs_finish, :libgmp), Void, (mpz_t, Clong), x, a) | ||
| import!(x::BigInt, a, b, c, d, e, f) = ccall((:__gmpz_import, :libgmp), Void, | ||
| (mpz_t, Csize_t, Cint, Csize_t, Cint, Csize_t, Ptr{Void}), x, a, b, c, d, e, f) |
There was a problem hiding this comment.
I prefer the previous style but well, I'm biased since I wrote it :-p
No description provided.