Michael Roth <mdr...@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
>>
