[squid-dev] [PATCH] Coverity(-inspired) fixes part four, HttpHeader refactor
Kinkie
gkinkie at gmail.com
Sun Aug 23 22:01:58 UTC 2015
.. now with the actual patch
On Mon, Aug 24, 2015 at 12:01 AM, Kinkie <gkinkie at gmail.com> wrote:
>
>
> 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
>
--
Francesco
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150824/f902b49f/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: coverity-fixes-4-v4.patch
Type: application/octet-stream
Size: 213614 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150824/f902b49f/attachment-0001.obj>
More information about the squid-dev
mailing list