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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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);
        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
>