[squid-dev] [PATCH] Coverity(-inspired) fixes part four, HttpHeader refactor

[Kinkie]

On Mon, Aug 24, 2015 at 2:10 PM, Amos Jeffries <squid3 at treenet.co.nz> wrote:

> On 24/08/2015 1:14 p.m., Alex Rousskov wrote:
> > On 08/23/2015 04:01 PM, Kinkie wrote:
> >>     Adding comment: "will reallocate if ilen happens to equal pool size"
> >
> > I would recommend not adding a comment at all if "no comment" and the
> > above "low-level description of the current c_str logic" are the two
> > choices, but it is your call. Not important.
>
> Agreed.
>
> Which is also why I just suggested the terse "performance regression,
> c_str() reallocates" in all cases where it *can*. Removal of those cases
> is still important (thus X vs TODO) but not reasonable at current time.
>

The performance regression will go as soon as we get strList to generate
SBufs.


> >
> >>     example.cc:11:28: error: invalid operands to binary expression
> >>     ('foo' and 'foo')
> >>     std::int8_t var = foo::one | foo::four;
> >>                       ~~~~~~~~ ^ ~~~~~~~~~
> >>     1 error generated.
> >
> > Thank you. We should not mix header IDs and header masks (a mask is not
> > an ID). I am not saying that you must change how Squid masks headers in
> > your patch, of course, but it does feel like the compiler has identified
> > a design flaw in our code. We should fix that at some point. It is your
> > call whether your patch is the right place for that fix.
>
> This is not about header IDs - thats a different alteration leading to
> this change. This bitmask thing is about enumerating the property flags
> that can be set in the table record. So we dont have to list false
> explicitly for all the irrelevant ones, or to depend on ordering.
>
>
> The design flaw I think is in use of enum class at all. Its just not
> suitable for bitmasks since the OR/AND-ing individual mask values does
> not produce a valid point-value within the defined set of enum class
> point-values. Wantig the compiler to reject that is exactly when enum
> class should be used.
>
> The basic c-style enum provides implicit type-casting to int_type which
> is what we need when using an enum to name bitmask bits. It does not
> necessarily mean badness. Just use with care. Its bad reputation is from
> people who didn't, or were forced to abuse it.
>
>
I am a bit wary of the implicit type-casting behavior, so I agree with Alex
that we should in time get rid of this pattern. Just not now, we have more
than enough stuff on our plate.


Am I cleared to commit? The comments to my latest patch submissions were
all about comments, and have been adopted.

Thanks


-- 
    Francesco
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150829/2b7fec27/attachment.html>