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

[Alex Rousskov]

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.


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


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


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


>     > +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]):


> enum class HdrKind: std::uint8_t {
>     None = 0,
>     ListHeader = 1,
...
> };
> 
> class Foo {
> public:
> 	HdrKind bar: 8;
> };

[1]
http://stackoverflow.com/questions/22606392/is-this-warning-related-to-enum-class-size-wrong



> "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 :-).


Thank you,

Alex.