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

[Kinkie]

On Sun, Aug 23, 2015 at 7:55 PM, Alex Rousskov <
rousskov at measurement-factory.com> wrote:

> On 08/21/2015 11:31 AM, Kinkie wrote:
> > On Fri, Aug 21, 2015 at 6:39 PM, Alex Rousskov wrote:
> >
> >     On 08/21/2015 04:37 AM, Kinkie wrote:
> >
> >     >   the attached patch does:
> >
> >     ... a long list of mostly unrelated changes ...
> >
> >     Sigh. Some of the patch changes are not necessary at all. Some are
> >     useful. Some should be redone to minimize conflicts. Some are
> >     performance optimizations that should be considered very carefully.
> It
> >     would take too much time to sift through all of this and separate the
> >     good from the bad, so we would have to accept pretty much all of
> them,
> >     create a lot of conflicts, and probably miss bugs.
>
>
> > To defend the code, ...
>
> Your code was not under attack. Your choice to assemble many unrelated
> and/or supposed-to-be-sequential changes into one patch was.
>

I'm sorry about that. I tend to go depth-first, unfortunately.
I'll try to resist, but it's really hard for me.


> >     > -        Tolower(name);
> >     > -
> >     > -        if (strcmp(name, "*") == 0) {
> >     > -            /* Can not handle "Vary: *" withtout ETag support */
> >     > -            safe_free(name);
> >     > +        SBuf name(item, ilen);
> >     > +        if (name.cmp("*",1) == 0) {
> >     >              vstr.clean();
> >     >              break;
> >     >          }
> >     > -
> >     > -        strListAdd(&vstr, name, ',');
> >     > +        name.toLower();
> >
> >     Do you know why we need to convert header names to lowercase here?
> The
> >     header names are not a part of some Store hash, right? This
> conversion
> >     is expensive because most header names will be changed here, breaking
> >     SBuf reuse. If you know the reason, let's add a comment since you are
> >     changing this code anyway.
>
>
> > Since lowerCase("*") == "*", I'd hope that the behavior is unchanged,
>
> I did not say something has changed. I asked *why* we need to convert
> header names to lower case. I am not trying to sneak in some veiled
> attack on your code inside an innocent-looking question. I asked exactly
> what I wanted to know. An "I do not know" would be a perfectly
> acceptable response, of course, but if you do know, it would be good to
> add a comment.
>

You are right, I got defensive over nothing.
Truth is, I don't know.


>
>
> >       // XXX: will reallocate if toLower() above is removed
> >
> >
> > .. and strListGetItem will start emitting SBufs in place of (char*,
> > length).
>
> Right, so perhaps s/will/may/ to be less precise since there are many
> factors here. The toLower() call is the most hidden factor though. This
> is not important -- if you disagree that a comment would be useful, do
> not add it. This is not an attack on your code.
>

Adding comment: "will reallocate if ilen happens to equal pool size"


>
>
> >     > -    while ((e = getEntry(&pos))) {
> >     > -        if (e->id == id)
> >     > -            delAt(pos, count);
> >     > -    }
> >     > +    int sz_before = entries.size();
> >     > +    entries.erase(std::remove_if(entries.begin(), entries.end(),
> >     > +    [=](HttpHeaderEntry *e) { return e && e->id == id; }),
> >     > +    entries.end());
> >
> >     This change is a performance regression AFAICT: Unlike the old
> delAt(),
> >     std::remove_if is very expensive for std::vector. Consider using
> >     std::replace_if without .erasing instead.
> >
> >
> > yes, it is a small regresson (from N to 2N)
>
> I suspect a factor of 2 does not describe the overhead accurately. If
> remove_if removes a field, then all subsequent fields (except those that
> are also removed) would have to be moved, which is probably much more
> expensive than just checking their values twice.
>
>
> > ). Reimplemented using replace_if - readability goes a bit down the
> > drain, unfortunatley.
>
> Perhaps a little bit. Personally, I could not interpret the earlier
> erase(remove_if) combination without looking things up anyway :-).
>
>
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 :)



> >     > +enum HdrKind {
> >
> >     Use C++11 class enums for global enums, especially when such use does
> >     not increase the amount of old code changes. In this specific case,
> it
> >     does not AFAICT.
> >
> >
> > Unfortunately it's not possible in ordeer to use it as a bit-field (I
> > have tried and the compiler refused)
>
> Interesting! If you still have an example of what goes wrong, can you
> post it? I tried a few tests myself, but they seem to be working fine if
> I specify the underlying class storage type (as we should, per [1]):
>

Using clang on MacOS:
------------------------ source code
#include <iostream>

enum class foo : std::int8_t {
  zero = 0,
  one = 1,
  two = 1<<1,
  three = 1<<2,
  four = 1<<3
};

std::int8_t var = foo::one | foo::four;

int main() {
  std::cout << var << std::endl;
  return 0;
}
---------------------------- compiler output
example.cc:11:28: error: invalid operands to binary expression ('foo' and
'foo')
std::int8_t var = foo::one | foo::four;
                  ~~~~~~~~ ^ ~~~~~~~~~
1 error generated.
 -----------------------------



>
>
> > "bzr send" unfortunately doesn't allow to specify context to be added :(
>
> Yes, that is unfortunate indeed. Fortunately, "bzr send" is not the only
> way to post a patch for review :-).
>

Yes :)
Attached current patch.
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'll
try to come up with a script to do blanket HERE removal as said elsewhere
in this thread).

Thanks!

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