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

Update deprecated ccall syntax in arpack #23307

Merged
merged 1 commit into from
Aug 21, 2017
Merged

Conversation

musm
Copy link
Contributor

@musm musm commented Aug 17, 2017

No description provided.

ido, bmat, &n, evtype, &nev, TOL, resid, &ncv, v, &ldv,
iparam, ipntr, workd, workl, &lworkl, rwork, info)
(Ref{BlasInt}, Ptr{UInt8}, Ref{BlasInt}, Ptr{UInt8}, Ref{BlasInt},
Ptr{$TR}, Ptr{$T}, Ref{BlasInt}, Ptr{$T}, Ref{BlasInt},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to me that all of these should be Ref not Ptr? From https://docs.julialang.org/en/latest/manual/calling-c-and-fortran-code/#Some-Examples-of-C-Wrappers-1

Note that for this code to work correctly, result_array must be declared to be of type Ref{Cdouble} and not Ptr{Cdouble}. The memory is managed by Julia and the Ref signature alerts Julia's garbage collector to keep managing the memory for result_array while the ccall executes. If Ptr{Cdouble} were used instead, the ccall may still work, but Julia's garbage collector would not be aware that the memory declared for result_array is being used by the external C function. As a result, the code may produce a memory leak if result_array never gets freed by the garbage collector, or if the garbage collector prematurely frees result_array, the C function may end up throwing an invalid memory access exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't really matter when used correctly.

@musm
Copy link
Contributor Author

musm commented Aug 18, 2017

part of #6080

@musm
Copy link
Contributor Author

musm commented Aug 20, 2017

G2g? Maybe benchmarks?

@fredrikekre
Copy link
Member

Didn't find any ARPACK benchmarks in the suite, but better safe than sorry
@nanosoldier runbenchmarks(ALL, vs=":master")

@musm
Copy link
Contributor Author

musm commented Aug 20, 2017

Thanks

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@musm
Copy link
Contributor Author

musm commented Aug 21, 2017

g2g?

@andreasnoack andreasnoack merged commit 2c915ab into JuliaLang:master Aug 21, 2017
@musm musm deleted the arp branch August 21, 2017 21:13
@musm
Copy link
Contributor Author

musm commented Aug 21, 2017

part of #6080 (ref)

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.

5 participants