From: David Michael Barr <david.barr@cordelta.com>
Date: Sat, 29 May 2010 10:55:24 GMT
Subject: Re: [PATCH 3/7] Add buffer pool library
Message-ID: <512B4EA0-463F-4CF2-94AB-404BF1BAE229@cordelta.com>
In-Reply-To: <20100529085124.GA6847@progeny.tock>

Ji Jonathan,

>> line_buffer creates a couple of static buffers and expose an API for
>> using them.
> 
> So this provides a thread-unsafe fgets() and fread() where the caller
> does not have to supply a buffer.  Sounds convenient.

Very convenient indeed. Its primary reason for existence is to assist
debugging the svn dump parser.

> Missing From: line and sign-off.

I'm sure Ram will fix this up in the next rebase.

>> +char *buffer_read_line(void)
>> +{
>> +    char *end;
> 
> style nitpick: use tabs to indent.

Will read git style guide and try to match for convenience.

> Why not fgets()?

Historical reasons, can be factored out.

> So if this buffer library is in use, all input needs to pass through
> it?  I would prefer to avoid that if possible.

The design is almost single-caller anyway...

>> +        line_len += offset;
>> +    }
>> +    while (offset < len && !feof(stdin)) {
>> +        offset += fread(&s[offset], 1, len - offset, stdin);
>> +    }
> 
> On error, wouldn’t this be an infinite loop?  Maybe:
> 
>  offset += fread(&s[offset], 1, len - offset, stdin);
>  if (ferror(stdin)) {
> 	free(s);
> 	return NULL;
> }
> 
> One iteration should be sufficient, since fread loops internally

Thanks for the tip.

--
David Barr