On Oct 17, 2011, at 4:58 PM, Peter Hutterer wrote:
>> + if (new_nevents <= eventQueue->nevents)
>> + return FALSE;
>> +
>> + if (!eventQueue) {
>> + ErrorF("[mi] mieqGrowQueue called with a NULL eventQueue\n");
>> + return FALSE;
>> + }
>
> this condition needs to be swapped with the one above it, otherwise you get
> a NULL dereference
><
>> +
>> + if (CountBits((const uint8_t *)&new_nevents, sizeof(new_nevents) != 1))
>> {
>
> this looks like a misplacement of a parens. Plus, this requires a comment as
> for the allowed values for nevents. Just add some documentation to
> mieqGrowQueue(), it would make reading the code a bit easier.
Yeah, it's not a requirement. I'm just nuking that line.
>> + }
>> +
>> + if (eventQueue->nevents) {
>> + /* % is not well-defined with negative numbers... sigh */
>> + n_enqueued = miEventQueue.tail - miEventQueue.head +
>> eventQueue->nevents;
>
> shouldn't this be using eventQueue?
>< Yes. I missed that one, thanks.
>> + &eventQueue->events[miEventQueue.head],
>
> shouldn't this be using eventQueue?
And that one.
> _please_ write a test. It's a self-contained function and these
> types of errors are found very easily with a few simple for loops and
> assert.
Yeah, I will.
>> + if ((miEventQueue.tail - miEventQueue.head) % miEventQueue.nevents >=
>> (miEventQueue.nevents >> 1) &&
>
> Wouldn't this suffer from the same issue regarding % on negative numbers?
Yeah, that one was already fixed after Keith's comments, but I didn't send an
updated patch yet.
> also, please put comments in here. I had to stare at this for too long to
> understand what you're doing here. a simple "grow queue when we have more
> than half the queue size queued up" would have made this a bit easier.
Ok, I'll spruce it up a bit.
> (I also don't know why >> 1 is preferable to /2. Not really critical path
> here anyway and /2 or * 2 would be much more obvious to read)
Habbit.
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
