Remove &x convention in ccall in mpfr
#23288
Conversation
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
base/mpfr.jl
Outdated
| if x < 0 | ||
| throw(DomainError(x, string($f, " will only return a complex result if called ", | ||
| "with a complex argument. Try ", $f, "(complex(x))."))) | ||
| if x < 0 && throw(DomainError(x, string($f, " will only return a complex result if called ", |
There was a problem hiding this comment.
This seems worse than before. What prompted this change?
There was a problem hiding this comment.
This is how it's styled on https://github.com/musm/julia/blob/1e86c79d537e80af750d436190d4d56db7e53504/base/mpfr.jl#L586
There was a problem hiding this comment.
Change the other one then?
There was a problem hiding this comment.
I don't understand, the one linked uses if x < -1, i.e. a normal if block? Also did you mean to remove the if here? otherwise this form if cond && throw(...) end is very strange.
|
part of #6080 (ref) |
base/mpfr.jl
Outdated
| throw(InexactError(:convert, Bool, x)) | ||
| function convert(::Type{Bool}, x::BigFloat) | ||
| x == 0 ? false : x == 1 ? true : throw(InexactError(:convert, Bool, x)) | ||
| end |
There was a problem hiding this comment.
Is this preferred (more readable) over the short form previously?
There was a problem hiding this comment.
x == 0 && return false
x == 1 && return true
throw(InexactError(:convert, Bool, x))
is more clear to me.
There was a problem hiding this comment.
Yeah that's much better
|
merge? |
base/mpfr.jl
Outdated
| if x == 0; return zero(BigInt) // one(BigInt); end | ||
| isnan(x) && return zero(BigInt) // zero(BigInt) | ||
| isinf(x) && return copysign(one(BigInt),x) // zero(BigInt) | ||
| x == 0 && return zero(BigInt) // one(BigInt) |
There was a problem hiding this comment.
As you already changed such a test to iszero earlier, and you are touching this function, you might as well use iszero here too?
There was a problem hiding this comment.
Let me change that back, I have another PR to make iszero isone uniform in the file.
| return (s, c) | ||
| end | ||
| sincos(v::BigFloat) = sincos_fast(v) | ||
|
|
||
| # return log(2) | ||
| function big_ln2() | ||
| c = BigFloat() | ||
| ccall((:mpfr_const_log2, :libmpfr), Cint, (Ptr{BigFloat}, Int32), | ||
| &c, MPFR.ROUNDING_MODE[]) | ||
| ccall((:mpfr_const_log2, :libmpfr), Cint, (Ref{BigFloat}, Int32), c, MPFR.ROUNDING_MODE[]) |
There was a problem hiding this comment.
Maybe this one was split on 2 lines to respect the 92 char limit? but anyway, your change is consistent with the rest of the file...
| return z | ||
| end | ||
| end | ||
|
|
||
| function log1p(x::BigFloat) | ||
| if x < -1 | ||
| throw(DomainError(x, string("log1p will only return a complex result if called ", | ||
| "with a complex argument. Try log1p(complex(x))."))) | ||
| "with a complex argument. Try log1p(complex(x))."))) |
There was a problem hiding this comment.
I think the previous version is better here, as the last line is an argument to string, not to DomainError.
There was a problem hiding this comment.
I had it before as you suggested, but @KristofferC said it looked strange so I changed it. I personally don't care either way and don't think it's a big deal, but will change according to a consensus..
There was a problem hiding this comment.
I didn't see his comment, but it's unrelated to ccall... it doesn't appear to be a common indentation style, so I would keep it as it was.
There was a problem hiding this comment.
See #23288 (comment)
in the file there are 2 different conventions on how this is broken up. I chose one of the two to make it consistent.
There was a problem hiding this comment.
Ah I see; but you had a typo there, maybe he was reacting to it, rather than the alignment?
There was a problem hiding this comment.
@KristofferC can you confirm that you support this modification of indentation? I cannot see enough lines of context in your older comment on an outdated diff to be sure.
|
I didn't follow the |
mbauman
added
the
kind:deprecation
|
This should now be ready to merge |
|
Merge? |
No description provided.