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

[Anthony Liguori <aliguori@us.ibm.com>]

Michael Roth <mdroth@linux.vnet.ibm.com> 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.

Regards,

Anthony Liguori

>         uint8_t cmos_data[128];
>         uint8_t cmos_index;
>         int32_t base_year qidl(property, "base_year", 1980);
>         uint64_t base_rtc;
>         uint64_t last_update;
>         int64_t offset;
>         qemu_irq irq qidl(immutable);
>         qemu_irq sqw_irq qidl(immutable);
>         int it_shift qidl(immutable);
>         /* periodic timer */
>         QEMUTimer *periodic_timer;
>         int64_t next_periodic_time;
>         /* update-ended timer */
>         QEMUTimer *update_timer;
>         uint64_t next_alarm_time;
>         uint16_t irq_reinject_on_ack_count qidl(broken);
>         uint32_t irq_coalesced;
>         uint32_t period;
>         bool has_coalesced_timer;
>         QEMUTimer *coalesced_timer qidl(optional);
>         Notifier clock_reset_notifier qidl(broken);
>         LostTickPolicy lost_tick_policy qidl(immutable) \
>             qidl(property, "lost_tick_policy", LOST_TICK_DISCARD);
>         Notifier suspend_notifier qidl(broken);
>     };
>
> b) current simplified form:
>     QIDL_DECLARE(RTCState) {                                                                            
>         ISADevice dev qImmutable;
>         MemoryRegion io qImmutable;
>         uint8_t cmos_data[128];
>         uint8_t cmos_index;
>         int32_t base_year qProperty("base_year", 1980);
>         uint64_t base_rtc;
>         uint64_t last_update;
>         int64_t offset;
>         qemu_irq irq qImmutable;
>         qemu_irq sqw_irq qImmutable;
>         int it_shift qImmutable;
>         /* periodic timer */
>         QEMUTimer *periodic_timer;
>         int64_t next_periodic_time;
>         /* update-ended timer */
>         QEMUTimer *update_timer;
>         uint64_t next_alarm_time;
>         uint16_t irq_reinject_on_ack_count qBroken;
>         uint32_t irq_coalesced;
>         uint32_t period;
>         bool has_coalesced_timer;
>         QEMUTimer *coalesced_timer qOptional;
>         Notifier clock_reset_notifier qBroken;
>         LostTickPolicy lost_tick_policy qImmutable \
>             qProperty("lost_tick_policy", LOST_TICK_DISCARD);
>         Notifier suspend_notifier qBroken;
>     };
>
> c) proposed simplified form:
>     QIDL_DECLARE(RTCState) {                                                                            
>         ISADevice dev q_immutable;
>         MemoryRegion io q_immutable;
>         uint8_t cmos_data[128];
>         uint8_t cmos_index;
>         int32_t base_year q_property("base_year", 1980);
>         uint64_t base_rtc;
>         uint64_t last_update;
>         int64_t offset;
>         qemu_irq irq q_immutable;
>         qemu_irq sqw_irq q_immutable;
>         int it_shift q_immutable;
>         /* periodic timer */
>         QEMUTimer *periodic_timer;
>         int64_t next_periodic_time;
>         /* update-ended timer */
>         QEMUTimer *update_timer;
>         uint64_t next_alarm_time;
>         uint16_t irq_reinject_on_ack_count q_broken;
>         uint32_t irq_coalesced;
>         uint32_t period;
>         bool has_coalesced_timer;
>         QEMUTimer *coalesced_timer q_optional;
>         Notifier clock_reset_notifier q_broken;
>         LostTickPolicy lost_tick_policy q_immutable \
>             q_property("lost_tick_policy", LOST_TICK_DISCARD);
>         Notifier suspend_notifier q_broken;
>
> Personally, I think b) is the only simplified form that reduces overall visual
> noise. So if b) isn't an option, I think a) is the way to go. Using the
> lowercasing for qidl(), and lack of an underscore that introduces an extra
> break, makes it a lot easier on the eyes, IMO. It's still more keystrokes, but
> that's not really my main concern: I just don't want to make all our devices
> a headache to look at.
>
>> 
>> Paolo
>>