[Libguestfs] [libnbd PATCH 2/4] generator: Add RUInt64 for 64-bit counters without error

[Laszlo Ersek]

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