Re: [Qemu-devel] [PATCH v2] Add infrastructure for QIDL-based device serialization

[Kevin Wolf]

Am 25.09.2012 23:12, schrieb Anthony Liguori:
> Michael Roth <address@hidden> writes:
> 
>> On Tue, Sep 25, 2012 at 08:37:16AM +0200, Paolo Bonzini wrote:
>>> Il 24/09/2012 20:14, Michael Roth ha scritto:
>>>>>>>> I went with qUppercase because it avoids all the previous issues with
>>>>>>>> using leading underscores, and it's reserved in terms of QEMU coding
>>>>>>>> guidelines as far as I can tell (we generally require leading capital
>>>>>>>> for typedefs and lowercase for variable names, and can work around
>>>>>>>> exceptions on a case by case basis by using QIDL() or some other name).
>>>>>>>> I also had it as q_* for a bit but that didn't seem much better on the
>>>>>>>> eyes we looking at converted structures.
>>>>>>
>>>>>> It looks like Hungarian notation and very much unlike other QEMU code.
>>>>>> I'd use q_ or qidl_ prefix instead, or rather QIDL().
>>>>>>
>>>> I wanted some way to distinguish from other qemu code to avoid conflicts,
>>>> but i think q_* seems reasonable if we reserve the prefix via CODING_STYLE.
>>>> Then for conflicts outside our control we can either use a different name
>>>> for the annotations or use the long-form QIDL() style depending on the
>>>> circumstances.
>>>
>>> I'm not sure why we need two ways to say the same thing...  I know it's
>>> just bikeshedding to some extent, but I'd really like to standardize on
>>> a single form.
>>
>> QIDL() (or maybe qidl()) should be the One True Form. It's the
>> only one that provides both proper namespacing and can be used both for
>> simple annotations and for ones that take parameters.
>>
>> I guess the real question is whether or not it makes sense to provide
>> "shortcuts" for the more common annotations to avoid clutter. I've heard
>> it both ways, so it's hard to decide.
>>
>> So let's bikeshed a bit. Maybe to put things into perspective, we're looking
>> at (and I'm just gonna go ahead and switch the OTF to qidl() now so we're
>> looking at the best case scenarios for both, and include q_* as well):
>>
>> a) One True Form:
>>     QIDL_DECLARE(RTCState) {                                                 
>>                            
>>         ISADevice dev qidl(immutable);
>>         MemoryRegion io qidl(immutable);
> 
> Just like sparse is a "compiler", so is qidl.  We are free to use the
> '_' + lowercase prefix.
> 
>           ISADevice _immutable dev;
> 
> It's an established practice in wide-use.

Not commenting on the underscore, but you did one thing that I want to
support: Put the (q)_immutable in a place where it looks like a
qualifier. Not so important for the qidl(...) syntax, but with the
simplified forms I definitely like it better.

I think I would even have made it '(q)_immutable ISADevice dev;', but
having the field name last is what really matters for readability.

Kevin