Re: [PATCH] HttpHdrSc c++ refactoring

Hi all,
  it's been 13 days since the last comment on this thread.
May I assume go-ahead for commit?

On Fri, Nov 18, 2011 at 8:42 AM, Kinkie <gkinkie_at_gmail.com> wrote:
>>> +/**
>>> + * 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/).
>
> Done (although s/slen/count/ to match the
>
>> 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
>
> Done.
>
>>
>>
>>> -        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?
>
> No, it wasn't.
> Thanks for spotting that, and sorry for missing it if you asked already :(
>
>>>  class HttpHdrSc
>>>  {
>>>
>>>  public:
>>> +    bool parse(const String *str);
>>> +    ~HttpHdrSc();
>>> +    HttpHdrSc(const HttpHdrSc &);
>>
>> Please move the parse() declaration below constructors/destructors.
>
> Done.
>
>>> +    HttpHdrSc() {};
>>
>> Extra semicolon.
>
> Removed.
>
>>>>> >> +    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.
>
> Gah! Sorry.. Now done.
> New version attached.
>
> --
>     /kinkie

-- 
    /kinkie