[PATCH 4 of 6] revlog: add public CPython function to get parent revisions

Georges Racinet gracinet at anybox.fr
Wed Dec 12 06:17:15 EST 2018


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 <yuya at 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




More information about the Mercurial-devel mailing list