Re: [Qemu-devel] [PATCH v2 3/4] qapi: Correctly handle downstream extensions in more locations

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3200 bytes --]

On 04/29/2015 05:29 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Now that c_var() handles '.' in downstream extension names, fix
>> the generator to support such names as additional types, enums,
>> members within an enum, branches of a union or alternate, and
>> in arrays.
>>

>> -        qtype = qtype,
>> -        abbrev = de_camel_case(name).upper(),
>> -        enum = c_var(de_camel_case(key),False).upper())
>> +                     qtype = qtype,
>> +                     value = generate_enum_full_value("%sKind" %c_var(name),
>> +                                                      key))
>>
>>      ret += mcgen('''
>>  };
> 
> I fixed this one in my "[PATCH RFC 06/19] qapi: Use c_enum_const() in
> generate_alternate_qtypes()".
> 
> You picked just my "[PATCH RFC 02/19] qapi: Fix C identifiers generated
> for names containing '.'" into your series.  Would you mind picking all
> the related patches, i.e PATCH 02-07?
> 
>   qapi: Fix C identifiers generated for names containing '.'
>   qapi: Rename _generate_enum_string() to camel_to_upper()
>   qapi: Rename generate_enum_full_value() to c_enum_const()
>   qapi: Simplify c_enum_const()
>   qapi: Use c_enum_const() in generate_alternate_qtypes()
>   qapi: Move camel_to_upper(), c_enum_const() to closely related code

Sure, I can pull in more of your patches in my v3.


>> +++ b/scripts/qapi-visit.py
>> @@ -44,12 +44,13 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error *
>>                   c_type=type_name(type))
>>
>>  def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base = None):
>> +    assert field_prefix == ""
> 
> Makes me wonder why we have a field_prefix parameter.
> 
> fn_prefix is also always ""...

Hmm - looks like I was debugging whether the code was dead (by whether I
could trigger the assertion) and forgot to clean it up.  I'll split that
into a separate patch (I've already done another related cleanup that I
found while reviewing this code, in commit 6540e9f).


>> -def generate_visit_list(name, members):
>> +def generate_visit_list(name, members, builtin=False):
>> +    if not builtin:
>> +        name = c_var(name)
> 
> Fun.
> 
> c_var() does two things:
> 
> (a) it protects certain words if protect=True
> 
> (b) it maps funny characters to '_'.
> 
> When builtin, (a) is unwanted, and (b) does nothing.  That's why we need
> the conditional.
> 
> A possible alternative could be c_var(name, not builtin).  Matter of
> taste.
> 
> Hmm, just saw what type_name() does.  Why not just
> 
>     name = type_name(name)
> 
> ?

Probably because I was just hacking until tests passed, rather than
trying to go for optimal code (after all, that's what the review process
is for, to find better ways to do things).  Sounds like a good cleanup
for v3.

> If it was my patch, I'd be tempted to split it up some.  Matter of
> taste, feel free to keep it a single patch.

I'll try and split it some for the next round.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]