On 11/17/2011 04:57 PM, Kinkie wrote:

> New iteration attached, please review it.

> +/**
> + * look for the last occurrence of a character in a c-string with a set 
> maximum length
> + */
> +SQUIDCEXTERN const char *strnrchr(const char *s, size_t slen, char c);

Please fix the strnrchr() description. To match the implementation, the
description should say that we scan from the beginning and stop at the
end of the c-string or n-th character, whichever comes first (and
s/slen/n/).

I would also recommend checking whether your strnrchr() implementation
matches that of GNU API. Their documentation is even worse:
  http://gnugeneration.com/mirrors/kernel-api/r2320.html


> -        if (!sctusable || sctusable->content.size() == 0)
> +        if (!sctusable || sctusable->hasContent())

Sorry if I asked you about this already, but is the reversal of the
second condition above intentional?


>  class HttpHdrSc
>  {
>  
>  public:
> +    bool parse(const String *str);
> +    ~HttpHdrSc();
> +    HttpHdrSc(const HttpHdrSc &);

Please move the parse() declaration below constructors/destructors.


> +    HttpHdrSc() {};

Extra semicolon.


>>> >> +    String Content() const { return content; }
>> >
>> > I would s/content/content_/ instead, especially since content data
>> > member is private. Capitalization should be used for static methods.
>
> Ok.

If that "Ok" meant "will do later", then please note that you have not
done that.


HTH,

Alex.

Reply via email to