ZMQ.jl icon indicating copy to clipboard operation
ZMQ.jl copied to clipboard

Generate complete bindings to libzmq

Open JamesWrigley opened this issue 1 year ago • 6 comments

This is a rather invasive change to use Clang.jl to generate bindings for everything in zmq.h, and then use those bindings in the rest of the library. My motivation for doing this is to make it easier to call the libzmq functions, both for us and for anyone who wants to use the low-level functions (e.g. for things we haven't explicitly wrapped). It's also nice to have the constants generated for us.

It touches a lot of internals so this would definitely benefit from a review from someone more familiar with them :sweat_smile: In particular the code for _Message and Message, which took me a while to understand. I've written down my understanding of how that works in the comments of gen.jl. For the reviewers: I've tried to keep the commits atomic, so it might be easiest to review the commits one-by-one. ~~I don't think it's quite ready to merge yet because I'd like to add some more docs, but it should be ready for review.~~

CC @stevengj, @vtjnash

JamesWrigley avatar May 20 '24 17:05 JamesWrigley

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 49.00990% with 103 lines in your changes missing coverage. Please review.

Project coverage is 74.90%. Comparing base (4e0e343) to head (cd4473b).

Files Patch % Lines
src/bindings.jl 29.85% 94 Missing :warning:
src/msg_bindings.jl 71.42% 8 Missing :warning:
src/comm.jl 75.00% 1 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #232       +/-   ##
===========================================
- Coverage   91.82%   74.90%   -16.93%     
===========================================
  Files           9       11        +2     
  Lines         367      526      +159     
===========================================
+ Hits          337      394       +57     
- Misses         30      132      +102     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar May 20 '24 17:05 codecov-commenter

Overall, I'm in favor of this, though I haven't had a chance to review it in detail.

stevengj avatar May 20 '24 17:05 stevengj

I made another change to the internals in 56d3a9b to force using Ref{_Message} everywhere. I think this is correct because it would be unsafe to use most/all of those functions with a plain _Message since it's a immutable struct and might not have a stable address.

JamesWrigley avatar May 21 '24 09:05 JamesWrigley

Added docs in 6729eba, and fixed the use of a deprecated function in ef30b30. Pending review, I think this is ready to merge :crossed_fingers:

(P.S., please don't merge it with the fixup commits, I'll rebase beforehand)

JamesWrigley avatar May 21 '24 15:05 JamesWrigley

Rebased the branch, and modified the generated bindings to use ccall() instead of @ccall in c13987d (for compatibility with Julia 1.3).

JamesWrigley avatar May 31 '24 11:05 JamesWrigley

(bump)

JamesWrigley avatar Jul 01 '24 12:07 JamesWrigley