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.

Sign up for GitHub

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

Jump to bottom

Add cfunction tuple of types deprecation #23066

Merged
merged 3 commits into from
Aug 23, 2017
Merged

Add cfunction tuple of types deprecation #23066

merged 3 commits into from
Aug 23, 2017

Conversation

musm
Copy link
Contributor

@musm musm commented Aug 1, 2017 •
edited
Loading

close #23020

@tkelman tkelman added kind:deprecation This change introduces or involves a deprecation needs news A NEWS entry is required for this change labels Aug 1, 2017
@fredrikekre fredrikekre removed the needs news A NEWS entry is required for this change label Aug 1, 2017
fredrikekre
fredrikekre reviewed Aug 1, 2017
View reviewed changes
base/deprecated.jl Outdated
@@ -1601,6 +1601,11 @@ end
# issue #6466
# `write` on non-isbits arrays is deprecated in io.jl.

function cfunction(f, r, a::Tuple)
Copy link
Member

Choose a reason for hiding this comment

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

could be simplified to

@deprecate cfunction(f, r, a::Tuple) cfunction(f, r, Tuple{a...})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, however I think the resulting deprecation warning message is not as helpful/clear in this case.

Copy link
Member

Choose a reason for hiding this comment

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

WARNING: cfunction(f, r, a::Tuple) is deprecated, use cfunction(f, r, Tuple{a...}) instead.

looks as helpful to me

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed, let the deprecate macro do what it's there for

@tkelman tkelman added the needs news A NEWS entry is required for this change label Aug 1, 2017
@fredrikekre
Copy link
Member

Did you lose the NEWS commit? I removed the label cause I saw you added that but now its not here?

@musm
Copy link
Contributor Author

musm commented Aug 1, 2017

hmm that is super strange...I will add it back.

@musm
Copy link
Contributor Author

musm commented Aug 2, 2017

Added back.

@tkelman tkelman removed the needs news A NEWS entry is required for this change label Aug 2, 2017
@tkelman
Copy link
Contributor

tkelman commented Aug 2, 2017

ah wondered what happened there

@musm
Copy link
Contributor Author

musm commented Aug 4, 2017

Github's rebase tools is strange. I had to rebase this twice, with the exact same changes to make it work.....

fredrikekre
fredrikekre reviewed Aug 5, 2017
View reviewed changes
base/deprecated.jl Outdated
@@ -1595,6 +1595,12 @@ end
# issue #6466
# `write` on non-isbits arrays is deprecated in io.jl.

# PR #23066
Copy link
Member

Choose a reason for hiding this comment

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

trailing white space

@musm
Copy link
Contributor Author

musm commented Aug 6, 2017

Unrelated Example install failures on appveyor and strange (unrelated) timeouts on travis

@fredrikekre
Copy link
Member

Failure is definitely related:

Expression: stderr == "MethodError: no method matching this_function_has_no_methods()\n"
   Evaluated: "MethodError: no method matching this_function_has_no_methods()\nWARNING: cfunction(f, r, a::Tuple) is deprecated, use cfunction(f, r, Tuple{a...}) instead.\nStacktrace:\n [1] depwarn at ./deprecated.jl:70 [inlined]\n [2] cfunction(::Function, ::Type{T} where T, ::Tuple{}) at ./deprecated.jl:57\nwhile loading no file, in expression starting on line 0\n" == "MethodError: no method matching this_function_has_no_methods()\n"
Stacktrace:
 [1] macro expansion at /private/tmp/julia/share/doc/julia/examples/embedding/embedding-test.jl:20 [inlined]
 [2] macro expansion at ./test.jl:943 [inlined]
 [3] anonymous at ./<missing>:?

@iblislin
Copy link
Member

iblislin commented Aug 6, 2017

:-o I should add the embedding testing to freebsd ci.

@musm
Copy link
Contributor Author

musm commented Aug 6, 2017

@fredrikekre good catch fixed!
@iblis17 👍

tkelman
tkelman approved these changes Aug 7, 2017
View reviewed changes
Copy link
Contributor

@tkelman tkelman left a comment

Choose a reason for hiding this comment

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

unrelated download failure on linux32, timeout (all tests were done though) on linux64, so lgtm - should definitely be squashed

@musm
Copy link
Contributor Author

musm commented Aug 14, 2017

merge ?

@musm musm force-pushed the dep branch 2 times, most recently from 148f997 to 7f68846 Compare August 18, 2017 06:54
@musm
Copy link
Contributor Author

musm commented Aug 18, 2017

squashed and rebased

@musm
Copy link
Contributor Author

musm commented Aug 21, 2017

is this good to go now?

@KristofferC
Copy link
Sponsor Member

KristofferC commented Aug 22, 2017 •
edited
Loading

The docs need updating? https://docs.julialang.org/en/latest/manual/calling-c-and-fortran-code/#Creating-C-Compatible-Julia-Function-Pointers-1

E.g julia> const mycompare_c = cfunction(mycompare, Cint, (Ref{Cdouble}, Ref{Cdouble}));

@musm
Copy link
Contributor Author

musm commented Aug 23, 2017

docs updated

@KristofferC KristofferC merged commit b330b0e into JuliaLang:master Aug 23, 2017
@musm
Copy link
Contributor Author

musm commented Aug 23, 2017

thanks @KristofferC

@musm musm deleted the dep branch August 23, 2017 20:22
@cormullion cormullion mentioned this pull request Mar 18, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredrikekre fredrikekre fredrikekre left review comments

@tkelman tkelman tkelman approved these changes

Assignees
No one assigned
Labels
kind:deprecation This change introduces or involves a deprecation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Regression calling cfunction
5 participants
@musm @fredrikekre @tkelman @iblislin @KristofferC