Re: [PATCH] HttpHdrSc c++ refactoring

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.

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

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

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

> /* put */
> + assert(mb.hasContent());
> addEntry(new HttpHeaderEntry(HDR_CACHE_CONTROL, NULL, mb.buf));

This change seems out of scope.

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

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

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

> === modified file 'tools/squidclient.cc'

Out of scope?

Thank you,

Alex.
Received on Fri Oct 28 2011 - 20:38:56 MDT