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.
