Laszlo Ersek
2022-Sep-08 12:53 UTC
[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
Eric Blake
2022-Sep-08 20:52 UTC
[Libguestfs] [libnbd PATCH 2/4] generator: Add RUInt64 for 64-bit counters without error
On Thu, Sep 08, 2022 at 02:53:21PM +0200, Laszlo Ersek wrote:> 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.Done in commit 92e7a396. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org