Re: [PATCH] HttpHdrSc c++ refactoring

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.
Received on Fri Nov 18 2011 - 02:26:48 MST