(resend 2)[2/5]hidclass.sys: Implement a report ring buffer

[Henri Verbeet]

On 10 July 2015 at 21:39, Aric Stewart <aric at codeweavers.com> wrote:
> +typedef struct __ReportRingBuffer
Did you borrow this from somewhere? Identifiers starting with double
underscore are reserved. (And using a single underscore instead isn't
much better.) The naming is also inconsistent with the existing code.
And I have to say that goes for much of the rest of the series as
well. There seems to be no consistent style from one patch to the
next, and sometimes within the same structure. (E.g. __WINE_ELEMENT,
__WINE_HID_REPORT, etc.) And of course most of this patch is unused
code until patch 5/5 when RingBuffer_Create() is actually called.

> +struct __ReportRingBuffer* RingBuffer_Create(UINT buffer_size)
> +{
> +    ReportRingBuffer *ring;
> +    ring = HeapAlloc(GetProcessHeap(), 0, sizeof(*ring));
> +    if (!ring)
> +        return NULL;
> +    ring->start = ring->end = 0;
> +    ring->size = 32;
> +    ring->buffer_size = buffer_size;
> +    ring->pointer_alloc = 5;
> +    ring->pointers = HeapAlloc(GetProcessHeap(), 0, sizeof(int) * 5);
> +    memset(ring->pointers, 0xff, sizeof(int) * 5);
> +    ring->buffer = HeapAlloc(GetProcessHeap(), 0, buffer_size * ring->size);
> +    InitializeCriticalSection(&ring->lock);
> +    ring->lock.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": RingBuffer.lock");
> +    return ring;
> +}
Why handle allocation failure for "ring", but not for "ring->pointers"
and "ring->buffer"? Why did you pick "5" for the initial size of
"ring->pointers"? Seems like an odd choice.

> +VOID RingBuffer_Destroy(struct __ReportRingBuffer *ring)
Why use "VOID"? Are you concerned "void" may not be available?

> +    if (size < 2 || size > 256 || size == ring->size)
> +        return;
What's special about 2 and 256? And if this does nothing, why would
you ever call the function like that?

> +    new_buffer = HeapAlloc(GetProcessHeap(), 0, ring->buffer_size * size);
> +    HeapFree(GetProcessHeap(), 0, ring->buffer);
> +    ring->buffer = new_buffer;
Why bother with "new_buffer" if you're not going to handle failures?