On Sun, Jan 20, 2013 at 5:56 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 5 January 2013 16:58, Magnus Hagander <magnus@hagander.net> wrote:
>> Attached is an updated version of the patch, per the comments from Tom
>> and rebased on top of the current master. Since it's been a long time
>> ago, and some code churn in the area, another round of review is
>> probably a good thing...
>>
>
> I took a look at this patch, and it seems to be in pretty good shape.
> It applies cleanly to head, and seems to work as advertised/discussed.
> I have a couple of comments on the code...
>
>
> In next_token(), in the case of an overlong token, this change looks wrong:
>
> /* Discard remainder of line */
> ! while ((c = getc(fp)) != EOF && c != '\n')
> ! ;
> break;
> }
>
> --- 188,195 ----
> errmsg("authentication file token too long, skipping: \"%s\"",
> start_buf)));
> /* Discard remainder of line */
> ! while ((c = (*(*lineptr)++)) != '\0' && c != '\n')
> ! (*lineptr)++;
> break;
>
> It appears to be incrementing lineptr twice per loop iteration, so it
> risks missing the EOL/EOF and running off the end of the buffer.
>
>
> Nitpicking, at the end of the loop you have:
>
> ! c = (**lineptr);
> ! (*lineptr)++;
>
> perhaps for consistency with the preceding code, that should be "c =
> (*(*lineptr)++)". Personally, I'd also get rid of the outer sets of
> brackets in each of these expressions and just write "c =
> *(*lineptr)++", since I don't think they add anything.
>
>
> Finally, the comment block for tokenize_file() needs updating, now
> that it returns three lists.
Thanks for the review - I've updated the patch per your comments
(minus the changing of the outer set of brackets - kept that the way
it was for consistency, but that can always be changed later if people
prefer that way), and will push it as it now stands.
-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
