Re: [PATCH] HttpHdrSc c++ refactoring

On 10/30/2011 01:17 PM, Kinkie wrote:

> Please find attached a new iteration of the patch for review.

> - temp = xstrndup (item, initiallen + 1);
> -
> - if (!((target = strrchr(temp, ';')) && !strchr (target, '"') && *(target + 1) != '\0'))
> + if (!((target = memrchr(item, ';')) && !strchr (target, '"') && *(target + 1) != '\0'))

I shortened the memchr() call in the new line for clarity -- it is not
relevant for this specific comment. Look at the stuff after "&&". The
old code was calling strchr() for "target" pointing to a zero-terminated
"temp" string. The new code calls it for target pointing to a
non-terminated "item". AFAICT, this means that we may find '"' beyond
the current item (at best). A bug?

> + case SC_MAX_AGE:
> + int ma;

IIRC, some compilers complain about a local variable not confined
"enough" to a single switch case scope (in case the if statements bypass
the "break" command and the compiler cannot reliably detect that?). I
tried to quickly find any examples of "bare" case-local variables in
Squid but failed.

Consider surrounding the whole case with {}.

HTH,

Alex.
Received on Mon Oct 31 2011 - 20:32:09 MDT