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

[Kinkie]

On Mon, Aug 24, 2015 at 3:14 AM, Alex Rousskov <
rousskov at measurement-factory.com> 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.
>

I'm removing the comment; the important thing is that we all agree that
this should not add any performance regression in the by-far common case
over the code it replaces.


> >     This is what it looks like right now:
> >
> >         //set all items in entries where item is not nullptr and
> >         // has same id as argument, to nullptr. Increment count variable
> >         // as a side-effect on matches during sweep
> >         std::replace_if(entries.begin(), entries.end(),
> >             [&](HttpHeaderEntry *e) {
> >                 if (e && e->id == id) {
> >                     ++count;
> >                     return true;
> >                 }
> >                 return false;
> >             },
> >             nullptr);
> >
> >     Hopefully the comment will clarify intent :)
>
> If you think this warrants a comment at all, consider just saying:
>
>     // replace matching items with nil and count them
>
> Again, if you think repeating your code using words is better, do that.
> Not important.
>

Using shorter variant.


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

It is a common use scenario where we use enums to define bitmasks. There
are for sure better alternatives, but this patch is not IMO the right place
where to address the issue.


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