Re: [PATCH v2 12/16] scripts/qapi: generate high-level Rust bindings

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 2/25/26 15:39, Markus Armbruster wrote:
>>> +def rs_name(name: str) -> str:
>>> +    """
>>> +    Map @name to a valid, possibly raw Rust identifier.
>>> +    """
>>> +    name = re.sub(r'[^A-Za-z0-9_]', '_', name)
>>> +    if name[0].isnumeric():
>> 
>> .isdigit()?  It's what c_name() uses...
>> 
>>> +        name = '_' + name
>> 
>> In review of v1, I pointed to "The Rust Reference"
>> 
>>         Identifiers starting with an underscore are typically used to
>>         indicate an identifier that is intentionally unused, and will
>>         silence the unused warning in rustc.
>> 
>>         https://doc.rust-lang.org/reference/identifiers.html
>> 
>> You replied "In this case it doesn't really matter: public items (such
>> as QAPI enum entries, or struct fields) do not raise the unused warning
>> anyway."
>> 
>> What gives us confidence rs_name() will only be used where it doesn't
>> really matter?
>
> The fact that all QAPI type definitions are (more or less by design) public.

Any particular reason not to use the same 'q_' prefix as in C?

>>> +    # avoid some clashes with the standard library
>>> +    if name in ('String',):
>>> +        name = 'Qapi' + name
>> 
>> This hides the unwise use of 'String' in qapi/net.json from Rust.  I'd
>> rather rename that one.
>
> Ok, BoxedString?

Works for me.

>>> +
>>> +    return name
>>> +
>>> +
>>> +def to_camel_case(value: str) -> str:
>>> +    return ''.join('_' + word if word[0].isdigit()
>>> +                   else word[:1].upper() + word[1:]
>>> +                   for word in filter(None, re.split("[-_]+", value)))
>> 
>> Please use r'...' for regular expressions always.
>> 
>> Why do you need filter()?
>
> To handle - or _ at the beginning or ending of a string, where an empty
> string would cause an IndexError in word[0].isdigit().

Got it, thanks.

>> This maps 'foo-0123-bar' to 'Foo_0123Bar'.  Intentional?  I'd kind of
>> expect 'Foo0123Bar'.
>
> Will fix (it is meant for 0123-45).  New version is:
>
>    def to_camel_case(value: str) -> str:
>        result = ''
>        for p in re.split(r'[-_]+', value):
>            if not p:
>                pass
>            elif p[0].isalpha() or (result and result[-1].isalpha()):
>                result += p[0].upper() + p[1:]
>            else:
>                result += '_' + p
>        return result

Maps '0123-45' to '_0123_45'.  Is the leading '_' intentional?

>>> +def mcgen(s: str, **kwds: object) -> str:
>>> +    s = mcgen_common(s, **kwds)
>>> +    return re.sub(r'(?: *\n)+', '\n', s)
>> 
>> This eats trailing spaces and blank lines.  The latter is a big hammer.
>> Without it, I see unwanted blank lines generated.  With it, I see wanted
>> blank lines eaten.  For instance:
>
> Ok, I can look into adding rstrip here and there.
>
>>> +        except FileNotFoundError:
>>> +            pass
>> 
>> This runs rustfmt to clean up the generated file.  Silently does nothing
>> if we don't have rustfmt.
>> 
>> Should we make rustfmt a hard requirement?  Please discuss this briefly
>> in the commit message.

I'm fine with not running rustfmt, and I'm fine with running it always
(makes it a hard requirement).  Running it sometimes feels like more
trouble than it's worth.

> It's unnecessary, but it does make the output look nicer.  If I add 
> rstrip, I don't need it.
>
>>> +    # Return the Rust type for common use
>> 
>> Are the uncommon uses?
>> 
>> There are for C types, and that's why we have both .c_type(),
>> .c_param_type(), nad .c_unboxed_type().
>
> Yes, Box<> and Vec<>.  They just don't deserve their own function unlike C.

I see.