-
Notifications
You must be signed in to change notification settings - Fork 6
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
Arbcall refactor #57
Arbcall refactor #57
Conversation
I'm happy with the changes, but @Joel-Dahne have a look at this as well |
@Joel-Dahne @kalmarek and myself are both happy with the changes and think that it's a good idea to always also generated the original signature as an escape hatch if our replacement logic is non-sensical. What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One issue with the automatic length is that you can still pass in two vectors with different lengths and crash Julia. So all of these methods should be considered unsafe. If we ever add exported versions of them we need to add proper bounds checks when wrapping them.
src/arbcall.jl
Outdated
# Automatic detection of length arguments for vectors | ||
elseif (startswith(string(name(carg)), "len") || carg == Carg{Clong}(:n, false)) && | ||
rawtype(cargs[i-1]) ∈ (ArbVector, AcbVector) && | ||
name(carg) ∉ len_keywords | ||
|
||
vec_name = name(cargs[i-1]) | ||
push!(kwargs, Expr(:kw, :($(name(carg))::Integer), :(length($vec_name)))) | ||
push!(len_keywords, name(carg)) | ||
|
||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we factor this out as a separate method? Mainly to make it easier to test independently since this is quite delicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I factored it out and added some tests
src/types.jl
Outdated
const ArbTypes = Union{ | ||
Arf, | ||
Arb, | ||
ArbRef, | ||
Acb, | ||
AcbRef, | ||
ArbVector, | ||
AcbVector, | ||
ArbMatrix, | ||
AcbMatrix, | ||
ArbRefVector, | ||
AcbRefVector, | ||
ArbRefMatrix, | ||
AcbRefMatrix, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor thing, but can we either keep the Ref
versions just under the default version or have all of them in the end, to make it easier to see directly what is going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
I did a very quick review of the code. In general I think it looks good, less code now. I think generating the original signature as well is reasonable! If we merge this then I can update #32 before merging that. |
This refactors the arbcall logic to fix #55 and to hopefully avoid things like this happening in the future.
This also adds some more features:
const ArbLike = Union{Arb,ArbRef,Ptr{arb_struct}}
.Int
and the previous argumentArbVectorLike
orAcbVectorLike
then the name of this argument is moved to be a kwarg, but only if the name isn
or starts withlen
.This also probably needs some more tests, but this not a task for 10pm :D