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

add keyword argument support to invoke #20345

Merged
merged 1 commit into from
Feb 2, 2017
Merged

add keyword argument support to invoke #20345

merged 1 commit into from
Feb 2, 2017

Conversation

JeffBezanson
Copy link
Member

Not the prettiest thing ever, but will get the job done.

fixes #7045

@JeffBezanson JeffBezanson added this to the 0.6.0 milestone Jan 31, 2017
jl_value_t *kws = jl_get_keyword_sorter(func);
JL_GC_PUSH1(&argtypes);
if (jl_is_tuple(argtypes)) {
jl_depwarn("`invoke(f, (types...), ...)` is deprecated, "
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume we don't need this depwarn?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately we do, since I transform the tuple type, so the following call to jl_f_invoke won't see that there was a tuple.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean this could just be an error since we never supported invoke with with a tuple of types and kwargs....

Copy link
Member

Choose a reason for hiding this comment

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

I think it's gentler to just leave this as a deprecation even though it could only happen if someone left an invoke(f, (t...), args...) call around and then added a keyword to it. Doesn't really hurt and we'll delete this in 1.0 anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure....

@JeffBezanson JeffBezanson merged commit 8f54aff into master Feb 2, 2017
@tkelman tkelman added the needs news A NEWS entry is required for this change label Feb 2, 2017
@tkelman tkelman deleted the jb/invokekw branch February 2, 2017 04:58
Sacha0 added a commit to Sacha0/julia that referenced this pull request May 12, 2017
@Sacha0 Sacha0 removed the needs news A NEWS entry is required for this change label May 13, 2017
tkelman pushed a commit that referenced this pull request May 14, 2017
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.

invoke does not accept keyword arguments
5 participants