On Mon, Sep 08, 2008 at 06:24:14PM +1000, Brendan Jurd wrote:
> On Sun, Sep 7, 2008 at 5:58 AM, Alex Hunsaker <badalex@gmail.com> wrote:
> > Im just following this:
> > http://wiki.postgresql.org/wiki/Reviewing_a_Patch so lets get started.
> >
>
> Hi Alex. Thanks for taking the time to review my patch.
I actually had a look at this patch also, though not as thoroughly as
Alex. There was one part that I had some thoughts about in from_char_parse_int_len:
! /*
! * We need to pull exactly the number of characters given in 'len' out
! * of the string, and convert those.
! */
<snip>
! first = palloc(len + 1);
! strncpy(first, init, len);
! first[len] = '\0';
The use of palloc/pfree in this routine seems excessive. Does len have
upper bound? If so a simple array will do it.
! if (strlen(first) < len)
Here you check the length of the remaining string and complain if it's
too short, but:
<snip>
! result = strtol(first, &last, 10);
! *src += (last - first);
Here you do not note if we didn't convert the entire string. So it
seems you are allowed to provide too few characters, as long as it's
not the last field?
Other than that it looks like a good patch.
Have a ncie day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.