On 17/10/2018 12:57, Artem Pisarenko wrote:
>> Further down in this patch the notation is QEMU_TIMER_ATTR_<id>, which I
>> think is clearer because QEMU_TIMER_ATTR(id) looks like a (non-existent)
>> macro. Please use the QEMU_TIMER_ATTR_<id> notation consistently.
>
> Yes, I've just forgot to update comments after previous patch version,
> where it actually was macro.
>
>> What is the purpose of this bit? I guess it's just here as a
>> placeholder because no real bits have been defined yet. Hopefully the
>> next patch removes it (/* This placeholder is removed in the next patch
>> */ would be a nice way to document this for reviewers).
>
> It's just to prevent compilation errors, as required by
> https://wiki.qemu.org/Contribute/SubmitAPatch#Split_up_long_patches
>
>> The enum isn't needed and makes debugging harder since the bit number is
>> implicit in the enum ordering. This alternative is clearer and more
>> concise:
>>
>> #define QEMU_TIMER_ATTR_foo BIT(n)
>
> Agree.
Like this?
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 86ce70f20e..ef7526e389 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -55,25 +55,13 @@ typedef enum {
/**
* QEMU Timer attributes:
*
- * An individual timer may be assigned with one or multiple attributes when
- * initialized.
- * Attribute is a static flag, meaning that timer has corresponding
property.
- * Attributes are defined in QEMUTimerAttrBit enum and encoded to bit set,
- * which used to initialize timer, stored to 'attributes' member and can be
- * retrieved externally with timer_get_attributes() call.
- * Values of QEMUTimerAttrBit aren't used directly,
- * instead each attribute in bit set accessed with QEMU_TIMER_ATTR_<id>
macro,
- * where <id> is a unique part of attribute identifier.
+ * An individual timer may be given one or multiple attributes when
initialized.
+ * Each attribute corresponds to one bit. Attributes modify the processing
+ * of timers when they fire.
*
* No attributes defined currently.
*/
-typedef enum {
- QEMU_TIMER_ATTRBIT__NONE
-} QEMUTimerAttrBit;
-
-#define QEMU_TIMER_ATTR__NONE (1 << QEMU_TIMER_ATTRBIT__NONE)
-
typedef struct QEMUTimerList QEMUTimerList;
struct QEMUTimerListGroup {
@@ -640,14 +628,6 @@ static inline QEMUTimer *timer_new_ms(QEMUClockType
type, QEMUTimerCB *cb,
return timer_new(type, SCALE_MS, cb, opaque);
}
-/**
- * timer_get_attributes:
- * @ts: the timer
- *
- * Return 0, or one to multiple OR'ed QEMU_TIMER_ATTR(id) values
- */
-int timer_get_attributes(QEMUTimer *ts);
-
/**
* timer_deinit:
* @ts: the timer to be de-initialised
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 2046b68c15..04527a343f 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -355,11 +355,6 @@ void timer_init_full(QEMUTimer *ts,
ts->expire_time = -1;
}
-int timer_get_attributes(QEMUTimer *ts)
-{
- return ts->attributes;
-}
-
void timer_deinit(QEMUTimer *ts)
{
assert(ts->expire_time == -1);
