[Mesa-dev] [PATCH v2] glsl: Add blob.c---a simple interface for serializing data

[Carl Worth]

On Fri, Dec 12 2014, Ian Romanick wrote:
> In addition to the (mostly) nits below, this seems ripe for some unit
> tests.  Especially for the overrun cases and the data alignment cases.

Thanks for the review, Ian. Testing makes sense. I'll cook something up.

And thanks for the nit-picking. I've cleaned those up, and I'll just
comment on one or two here:

>> +grow_to_fit (struct blob *blob, size_t additional)
>               ^ No space here or other similar places.

I'm happy to conform to Mesa style here, (and I've already changed the
patch).

But in passing, let me say that I've learned to really love having a
space between a function name and its left parenthesis, (in spite of it
looking totally bizarre at first). Give it a try in some small project
some time and see what you think. For me, it improves readability just
as much as the space we use between "if" and its parenthsis.

>> +   if (blob->allocated == 0)
>> +      to_allocate = BLOB_INITIAL_SIZE;
>
> Can additional ever be > BLOB_INITIAL_SIZE?

Good point. In the current code, no, but there's no reason to have this
fragility in place. So I've now got the MAX2 in place like this:

   if (blob->allocated == 0)
      to_allocate = BLOB_INITIAL_SIZE;
   else
      to_allocate = blob->allocated * 2;

   to_allocate = MAX2(to_allocate, blob->allocated + additional);

(That's the only real change in the functionality of the code, so I
won't bother re-sending a patch with the whitespace and comment fixes)

>> +/* When done reading, the caller can ensure that everything was consumed by
>> + * checking the following:
>> + *
>> + *   1. blob->data should be equal to blob->end, (if not, too little was
>> + *      read).
>
> blob->data or blob->current?

Yes, blob->current. A good fix.

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141212/7642ca16/attachment.sig>