On 12/9/18 6:02 AM, Yuya Nishihara wrote: > On Wed, 05 Dec 2018 22:43:35 +0900, Yuya Nishihara wrote: >> # HG changeset patch >> # User Yuya Nishihara <y...@tcha.org> >> # Date 1543756237 -32400 >> # Sun Dec 02 22:10:37 2018 +0900 >> # Node ID 716a73bab79be30c20c75e13324c44205d5e2120 >> # Parent 3842abba948cd7f4bb3fad6805265a35fb94a083 >> revlog: add public CPython function to get parent revisions >> +/* >> + * Get parents of the given rev. >> + * >> + * If the specified rev is out of range, IndexError will be raised. If the >> + * revlog entry is corrupted, ValueError may be raised. >> + * >> + * Returns 0 on success or -1 on failure. >> + */ >> +int HgRevlogIndex_GetParents(PyObject *op, int rev, int *ps) > This is based on the idea that the Rust module will be statically linked with > the cext objects. I thought that would be easier for our use case, > package-local > visibility, but I'm not certain. If that makes things complicated, maybe we > should choose dynamic linking and wrap the function with PyCapsule, as you did > in the PoC code.
Yes, it's true in the direct-ffi code, I passed the function pointer around because I was wary of a loop in dependencies (that's a bit silly since we can't avoid linking the Rust extension within the parsers module anyway), but also I didn't want to touch the existing C code too much for my first patch set. This version you're proposing feels simpler to me. Using a capsule in that context wouldn't be much complicated either, all we'd need in hg-direct-ffi would be to declare and call PyCapsule_Import (it's obviously designed to use very few CPython API concepts, only needs a char * for the name). The advantage it'd have then would be that the same capsule could be used for all types of FFI, and we could do the same for other functions we could need later on (maybe in other packages). Adding a new capsule seems less risky for people like me, who aren't as familiar with mercurial's C code as you are or, if you prefer to see it that way, will require less review. To be more explicit for other mailing-list subscribers, here's the change in revlog.c I'm doing in my PoC CPython code (this is inside the module init function) : @@ -2846,6 +2846,12 @@ if (nullentry) PyObject_GC_UnTrack(nullentry); + void *caps = PyCapsule_New( + index_get_parents, "mercurial.cext.parsers.index_get_parents_CAPI", + NULL); + if (caps != NULL) + PyModule_AddObject(mod, "index_get_parents_CAPI", caps); + #ifdef WITH_RUST rustlazyancestorsType.tp_new = PyType_GenericNew; if (PyType_Ready(&rustlazyancestorsType) < 0) So, to summarize, I think we should maybe promote capsules as the preferred way to interface Rust code with inner C code. It wouldn't be hard to document either. What do you think ? And yes, I should submit formally those rust-cpython bindings sooner than later. Regards, -- Georges Racinet Anybox SAS, http://anybox.fr Téléphone: +33 6 51 32 07 27 GPG: B59E 22AB B842 CAED 77F7 7A7F C34F A519 33AB 0A35, sur serveurs publics _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel