Re: [MERGE] c++-refactor HttpHdrCc

On 09/28/2011 07:23 AM, Kinkie wrote:

> Attached is version 6; run-tested.
> Includes StringArea.h this time.

Sorry, I found a few more bugs in this patch. They are detailed below,
together with a more polishing touches.

> + // default constructor is disabled
> + StringArea();

Remove. There is no generated default constructor for classes with other
constructors.

There are generated copy constructor, destructor, and assignment
operator, but they all do what we want them to do.

> +/// iterate over a table of http_header_cc_type structs
> http_hdr_cc_type &operator++ (http_hdr_cc_type &aHeader)

This operator does not know anything about tables. It simply increments
its argument.

> + inline void maxStale(int32_t newval) {
> + if (newval < 0 && newval != CC_MAX_STALE) return;

CC_MAX_STALE is an identifier, not a special value. Did you mean
MAX_STALE_UNKNOWN? And "||" instead of "&&"?

> + inline void maxAge(int32_t newval) {if (newval < 0) return; max_age = newval; setMask(CC_MAX_AGE); }
> + inline void sMaxAge(int32_t newval) {if (newval < 0) return; s_maxage = newval; setMask(CC_S_MAXAGE); }
> + inline void minFresh(int32_t newval) {if (newval < 0) return; min_fresh = newval; setMask(CC_MIN_FRESH); }
...

I think we should either assert/throw OR clear the mask when the value
is negative, but we should not silently ignore it.

Also, please s/newval/newVal/ or s/newval/v/.

I do not think you need to say "inline" when you are providing a method
definition at the time of the method declaration.

BTW, you may want to consider using one macro to define four standard
methods for integer-valued directives and another macro to define four
standard methods for boolean directives.

You can also add a setValue() or similar method that will implement the
above logic, given a reference to the data member and directive ID. It
is wrong to duplicate so much code! Sorry if I missed that in my earlier
sketch.

> + // max-stale has a special value (MAX_STALE_ALWAYS) which correspond to having
> + // the directive without a numeric specification, and directs to consider the object
> + // as always-expired.

Your always-expired definition is the opposite of what valueless
max-stale means, which is "the client is willing to accept a stale
response of any age."

s/MAX_STALE_ALWAYS/MAX_STALE_UNLIMITED/ or
s/MAX_STALE_ALWAYS/MAX_STALE_ANY/ (I will assume the latter in the
comments below).

You may want to set MAX_STALE_ANY to the maximum valid value. This
should not be necessary, but if there are bugs where we use max-stale
and do not check for MAX_STALE_ANY it will help mask some of them.

Add a boolean maxStaleAny() to avoid exposing the caller to your special
MAX_STALE_ANY constant.

You could also s/maxStale()/maxStaleFixed()/ to emphasize that the
caller should not trust maxStale() values blindly. Your call.

> + if (flag == CC_MAX_STALE && maxStale()!=MAX_STALE_ALWAYS)
> + packerPrintf(p, "=%d", (int) maxStale());

Why do we never pack MAX_STALE_ALWAYS? Either pack it or add a comment
explaining why it is parsed but never packed.

> + int32_t ma;
> + if (!p || !httpHeaderParseInt(p, &ma)) {

You did not add local variables for parsing other directives. Do not
make max-age special.

However, note that httpHeaderParseInt() needs an int, and not int32_t
parameter. This is one of the reasons why you probably should not change
the type of these values.

> - } else {
> - EBIT_SET(cc->mask, type);
> }
...
> + if (!p || !httpHeaderParseInt(p, &s_maxage)) {
> debugs(65, 2, "cc: invalid s-maxage specs near '" << item << "'");
> + clearSMaxAge();
> }
> break;

AFAICT, we are now missing a call to set the bit mask for s-maxage. Same
for many other directives! If I am right about this, please note that
your testing was pretty weak since it did not detect these bugs.

Finally, looking at the actual usage, I would s/haveFoo/hasFoo/ in the
new method names because "cache_control" is singular. "Has" is also
shorter :-).

Cheers,

Alex.
Received on Wed Sep 28 2011 - 16:03:03 MDT