Re: [Libguestfs] [libnbd PATCH 2/4] generator: Add RUInt64 for 64-bit counters without error
On 09/06/22 11:08, Eric Blake wrote:
On Mon, Sep 05, 2022 at 02:00:22PM +0200, Laszlo Ersek wrote:
We could further compress things to have:
pr " debug (h, \"leave: ret=\"";
(match ret with
| RBool -> pr "%%d\", ret"
and so on, to get rid of yet another spurious pair of "" in the
generated C code (since the return format is always exacty one
argument, there's no need to have the generated api.c have
"... ret=" "%<>" when "... ret=%<>" would do).
I can touch that
up in a followup, if it makes sense.
It seems to simplify the generated code (which we sometimes quote in
commit messages, when patching the generator), so if it's not a big
burden, I'd be in favor of it.
>> );
>> pr ");\n";
>> (match ret with
>> | RStaticString | RString -> pr " free
(ret_printable);\n"
>> | RBool | RErr | RFd | RInt
>> | RInt64 | RCookie | RSizeT
>> - | RUInt | REnum _ | RFlags _ | RUIntPtr -> ()
>> + | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> ()
>
> looks like there were multiple RUintPtr omissions, those could go into a
> unified (but from this, separate) patch.
Actually, this line rearranged RUIntPtr to be earlier in the line (we
don't have a strong precedent on whether branches to the match should
be in alphabetical, declaration, or abitrary order).
Yup, sorry. The reordering, combined with the insertion, threw me off.
>
>> );
>> pr " }\n";
>> pr " }\n"
>> @@ -904,6 +907,8 @@ let
>> pr "This call returns a bitmask.\n";
>> | RUIntPtr ->
>> pr "This call returns a C<uintptr_t>.\n";
>> + | RUInt64 ->
>> + pr "This call returns a counter.\n";
>
> Hmmm. On one hand, I like that here we do tie the new type to counters.
> On the other hand, the explanation doesn't match the generic name
> "RUint64". Not sure what to suggest :/
If we later find ourselves needing an actual 64-bit unsigned value,
unrelated to counters, we can add RCounter at that point and
repurpose RUInt64 to the new purpose. We've done that sort of enum
splitting before; the enum names are less important that the
underlying abstract type that then has to be ported to all the
language bindings for the semantics it will be representing.
OK.
>> +++ b/generator/Python.ml
>> @@ -525,7 +525,7 @@ let
>> | RErr ->
>> pr " py_ret = Py_None;\n";
>> pr " Py_INCREF (py_ret);\n"
>> - | RFd | RInt | REnum _ | RFlags _ | RSizeT | RUInt | RUIntPtr ->
>> + | RFd | RInt | REnum _ | RFlags _ | RSizeT | RUInt | RUIntPtr | RUInt64
->
>> pr " py_ret = PyLong_FromLong (ret);\n"
>> | RInt64 | RCookie ->
>> pr " py_ret = PyLong_FromLongLong (ret);\n"
>>
>
> This does not look right; I think RUInt64 belongs with RInt64 and
> RCookie (so that we invoke PyLong_FromLongLong()).
D'oh. Actual bug; now fixed as followup commit f79a1ac1
Thanks for reviewing.
Cheers
Laszlo