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

[Alex Rousskov]

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.


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


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


>     If I understand correclty, that the only point to clear before merge
>     is how many lines away from a touched line should a HERE be to be
>     removed

I do not have the time to argue about this super important [sarcasm]
issue: I have suggested and even implemented a simple and precise rule.
If others prefer something else, so be it. When we are wasting days
arguing about other stuff that should not be argued about, this minor
issue is just a blimp on the radar. A few HERE changes is _not_ the
reason to avoid committing your (or any other!) patch.


Cheers,

Alex.