On 12/11/18 2:10 PM, Yuya Nishihara wrote: > On Mon, 10 Dec 2018 18:54:56 +0100, Georges Racinet wrote: >> 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 ? > My only concern about using PyCapsule is, we would have to get around some > "static mut" variables in Rust if the cost of the name resolution matters. > That's what I noticed while reading your rust/hg-cpython code. I understand that concern. Would a protection with std::sync::Once be enough to address it? The official Rust doc (https://doc.rust-lang.org/std/sync/struct.Once.html) claims that the static mut is then actually safe. > > To be clear, I'm not against using PyCapsule. It's documented as the "right" > way to interface C-level APIs across modules. I wrote this patch before > reading your upcoming series. That's the only reason I made the GetParents() > function public.
About that upcoming series, I should be able to submit it real soon now: I don't consider https://github.com/dgrunwald/rust-cpython/issues/164 to be a blocker anymore. 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