On 10/26/2012 04:23 AM, Amos Jeffries wrote:
> Updated patch attached for review.
This seems to add a typo bug:
> + } else if (!strncmp(p,"ERR",3) && (p[3] == ' ' || p[3] == '\n' ||
> p[2] == '\0')) {
s/p[2]/p[3]/?
"buf" is guaranteed to be 0-terminated, right?
Should we be worried about \r before \n? Perhaps on Windows?
>>> + // check we have something to parse
>>> + if (!buf || len < 1)
>>> + return;
>> ...
>>> + if (len >= 2) {
>> Would not all the code work if len is zero or one? If yes, remove the
>> len checks, especially if len is going to be more than 1 in 99.9999% of
>> cases.
>
> URL-rewriter which are unfortunatley common still return empty response
> as their most common case.
> The cases where >= 2 will be closer to 50% of overall helper responses
> IMO. Unti that is gone this is an optimization to prevent multiple
> strncmp() being run on 0-1 length strings.
Understood. Please add a comment explaining this. For example,
// optimization: large portion of [URL-rewriter] responses are a single
// character (that we will stuff into other_).
if (len >= 2) ...
However, it feels wrong that we do not check what that character is (or
does the URL rewriting code do that?).
Cheers,
Alex.
