Re: [PATCH] HttpHdrSc c++ refactoring

On Fri, Oct 28, 2011 at 10:38 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 10/28/2011 09:55 AM, Kinkie wrote:
>
>
>> +    HttpHdrScTarget(const char *target_):
>> +        mask(0), max_age(MAX_AGE_UNSET), max_stale(MAX_STALE_UNSET),target(target_) {}
>> +    HttpHdrScTarget(const String &target_):
>> +        mask(0), max_age(MAX_AGE_UNSET), max_stale(MAX_STALE_UNSET),target(target_) {}
>
> Please add "explicit" to both constructors to avoid implicit conversions
> from strings to HttpHdrScTarget.
>
> BTW, do we really need the first constructor if there is already a
> silent conversion from c-string to a String? Try removing it.

Done, and done.

>> +    HttpHdrScTarget(const HttpHdrScTarget &t):
>> +        mask(t.mask), max_age(t.max_age), max_stale(t.max_stale),
>> +        content(t.content), target(t.target) {}
>
> Please add a comment why we do not copy the node field. If node should
> be copied, then remove the above as not needed.

This is a refactoring of httpHdrScTargetDup; I've checked, it doesn't
copy the node field. I don't know why.

> * It is kind of sad that HttpHdrScTarget copies a lot of HttpHdrCc code.
> I think we are missing a common directive-agnostic class between them.
> Not a big deal, I guess.

I partly agree. At the same time, they behave differently enough that
abstracting would be harder than keeping the duplication.

>> +    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.

>>      /* put */
>> +    assert(mb.hasContent());
>>      addEntry(new HttpHeaderEntry(HDR_CACHE_CONTROL, NULL, mb.buf));
>
> This change seems out of scope.

Right. Removed.

> It is strange that you did not add a similar assert to the corresponding
> HDR_SURROGATE_CONTROL code!

It _is_ out of scope; and it does highlight a potential bug. I'm
removing that, but we should probably consider adding that for all
HttpHeader::put* methods.

>> === modified file 'src/htcp.cc'
> ...
>> +#include "compat/xalloc.h"
>
> Please check that this is needed even if there are no other changes to
> src/htcp.cc.

Strange, I don't know where it came from.
It's not there.

>> -        if (!request->cache_control->Public()) {
>> +        if (!request->cache_control || !request->cache_control->Public()) {
>>              if (!REFRESH_OVERRIDE(ignore_auth))
>
> Seems out of scope (and already in trunk).

I'm basing over an old version of trunk while it's being stabilized;
I'll leave it in but will be removed upon merge.

>> === modified file 'tools/squidclient.cc'
>
> Out of scope?

Yes, but needed to compile with newer gcc (Oneiric kubuntu - I'm off
regular Ubuntu due to Unity brokenness).
Will be removed upon merge.

I'll leave in compat/strnrchr.{h,cc} as it's done and shouldn't hurt.
Please let me know if you want it removed.

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

Thanks!

-- 
    /kinkie