Stefan Hajnoczi writes:
> On Thu, Jun 09, 2016 at 04:17:11PM +0200, Lluís Vilanova wrote:
>> >> @@ -61,7 +69,7 @@ static inline bool
>> >> trace_event_get_state_static(TraceEvent *ev)
>> >> static inline bool trace_event_get_state_dynamic_by_id(TraceEventID id)
>> >> {
>> >> /* it's on fast path, avoid consistency checks (asserts) */
>> >> - return unlikely(trace_events_enabled_count) &&
>> >> trace_events_dstate[id];
>> >> + return unlikely(trace_events_enabled_count) &&
>> >> (trace_events_dstate[id] > 0);
>>
>> > typeof(trace_events_dstate[0]) is size_t, so trace_events_dstate[id] > 0
>> > is equivalent to trace_events_dstate[id] (due to unsigned). Why change
>> > this line?
>>
>> Sorry, I have a tendency to make this type of checks explicit when the types
>> are
>> not boolean (for a maybe-false sense of future-proofing). I can leave it as
>> it
>> was if it bothers you.
> When reviewing patches I try to understand each change. When I don't
> see a reason for a change I need to ask.
> In general it's easier to leave code as-is unless there is a need to
> change it. But there are no hard rules :).
I'll refrain from pushing my manias into QEMU :)
[...]
>> > The number of cpus has type int (see CPUState *qemu_get_cpu(int index)).
>>
>> > Why did you choose size_t?
>>
>> It just sounds proper to me to use size_t, since the state can never be
>> negative
>> (it's either interpreted as a boolean or as an unsigned counter, depending on
>> the "vcpu" property).
> If you feel strongly about it, feel free to keep it. Alternative
> reasoning about the type:
> int is the CPU index type used in qemu_get_cpu(). It is guaranteed to
> be large enough for the vcpu count. IMO there's no need to select a new
> type, but there's more...
> size_t is larger than necessary on 64-bit machines and has an impact on
> the CPU cache performance that Paolo's optimization takes advantage of
> (if you trigger adjacent trace event IDs they will probably already be
> in cache).
> size_t made me have to think hard when reading the "int += bool -
> size_t" statement for updating trace_events_enabled_count.
> If int is used then it's clear that int = (int)bool - int will be one of
> [-1, 0, +1].
> But with size_t you have to starting wondering whether the type coercion
> is portable and works as expected:
> int = (int)((size_t)bool - size_t);
> In "6.3.1.3 Signed and unsigned integers" the C99 standard says:
> [If] the new type is signed and the value cannot be represented in
> it; either the result is implementation-defined or an
> implementation-defined signal is raised.
> The size_t -> int conversion is therefore implementation-defined. This
> is not portable although QEMU probably does it in many places.
> So for these reasons, I think int is the natural choice.
Fair point. But now I feel tempted to change both trace_events_dstate and
trace_events_enabled_count into unsigned int... it burns me when I see signed
types used only on their positives by design.
But don't worry, I'll change trace_events_dstate into int :)
Thanks!
Lluis
