[squid-dev] [PATCH] Remove some splay users

[Kinkie]

On Tue, Jan 13, 2015 at 3:09 AM, Amos Jeffries <squid3 at treenet.co.nz> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 13/01/2015 8:51 a.m., Kinkie wrote:
>> Hello, the attached patch changes some users of splay to std::set.
>> The aim is to get more predictable (if not necessarily better)
>> performance and leverage a cleaner API resulting in more readable
>> code.
>>
>> This is a merge snapshot of branch lp:~squid/squid/replace-splay
>> revno 13835; I will continue working on the branch to migrate more
>> users.
>>
>> The touched classes have been build- and run-tested.
>>
>
>
> in src/acl/UserData.h, src/acl/Eui64.h, etc
> * still need to include splay.h in these .h files?
>   perhapse it can be reduced down to the relevant .cc or dropped entirely.

dropped.

> in src/acl/Eui64.cc:
> * ACLEui64::parse() can be optimized a bit further:
>
>    while (const char *t = ConfigParser::strtokFile()) {
>     if (Eui::Eui64 *q = aclParseEuiData(t)) {
>        ... what used to follow the if
>        xfree(q);
>     }

done, although it is not a performance-critical path.

>   }
>
> * ACLEui64::dump() buf variable can remain as static. It's only used
> until SBuf data-copies, no need to re-allocate on each loop.

Ok

>  - also while changing, if possible use SZ_EUI64_BUF otherwise
> document why magic number is 48 used. (EUI uses 3 readable characters
> to represent each octet of the number - buf is approx. double the
> required size).

SZ_EUI64_BUF is 8; I understand that's for the binary representation,
not textual.
I kept the buffer as it was, after all it's not huge anyway.

>
>
> in src/acl/UserData.cc:
> * #include SbufAlgos.h before util.h

Done.

> * for ctor/dtor like ACLUserData::~ACLUserData() which have become
> NOP, please inline them in the .h where possible. That allows some
> more compiler optimizations.

Is that even true when they are virtual? But sure, it means fewer
lines of code so that's OK.

> * is cend() usable instead of end() for the find test in the function
> above ACLUserData::dump() [ chunk @55, I think its probably the end of
> match()]

Using const iterators in the copied-from container.

> * why is CaseInsensitveSBufCompare() not defined in SBufAlgos.h for
> re-use ?

Should I do that as part of this branch or merge separately?