Re: [PATCH 3/7] Add buffer pool library
- From
David Michael Barr <david.barr@cordelta.com>
- Date
- May 29, 2010, 10:55 UTC
- Message-ID
- <512B4EA0-463F-4CF2-94AB-404BF1BAE229@cordelta.com>
- In-Reply-To
- <20100529085124.GA6847@progeny.tock>
Ji Jonathan,
Show 5 quoted lines
>> 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.
Show 5 quoted lines
>> +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...
Show 15 quoted lines
>> + 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 internallyThanks for the tip.
-- David Barr