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

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.

>> +    # 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?

>> +
>> +    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().

> 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


>> +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.

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.

Paolo