<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Jul 26, 2015 at 10:23 PM, Amos Jeffries <span dir="ltr"><<a href="mailto:squid3@treenet.co.nz" target="_blank">squid3@treenet.co.nz</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 27/07/2015 4:48 a.m., Kinkie wrote:<br>
</span><span class="">> HI,<br>
>   the low-hanging fruits from Coverity's analysis have been picked, now<br>
> working on somewhat more complex fixes.<br>
> The attached patch takes a hint from two benign coverity defects to:<br>
<br>
</span><span class="">> - refactor Digest auth's field lookup table to use a std::map instead of<br>
> abusing httpHeaderFieldsInfo; this improves readability and size of the<br>
> code, and has the added small bonus of lowering the lookup cost from linear<br>
> to logarithmic<br>
><br>
> the Digest code has been run-tested. I'd like feedback on its style, as<br>
> httpHeaderFieldsInfo is abused similarly elswehere and I'm considering to<br>
> apply it elsewhere as well; it can then be further refined to get O(1) via<br>
> a carefully-chosen hash (via std::unsorted_map)<br>
><br>
<br>
</span>My thoughts: (sorry if its a bit rambling)<br>
<br>
I dont like spawning lots of new classes for basic things. I know its<br>
essentially the C++ way. But applying pattern theory can go too far<br>
sometimes. And this patterns usage is one case where I can see exactly<br>
that happening.<br>
<br>
You have a struct, a class, an enum and array. Thats a lot of custom<br>
infrastructure just to represent a simple set of name:id. We could as<br>
easily have std::map<const char*, enum> holding that directly and avoid<br>
all the local types except enum.<br>
<br>
There are already 5 headers currently in Squid that need these same<br>
operations applied, and at least as many more that should but dont even<br>
use the current HttpHeaderFieldAttrs related types yet.<br>
<br>
<br>
<br>
struct HttpDigestFieldAttrs should be a template class so we dont have<br>
to re-implement a new little struct for each enum.<br>
<br>
BUT, notice that generalizing that same structure is also where the enum<br>
casting hack for HttpHeaderFieldAttrs comes from in the first place.<br>
 The probkem was template style breaks with C++03 compilers thinking all<br>
enums are of "int" type. Enter lots of repeated-instantiation complaints<br>
from dumb compilers.<br>
 I've not tried it with current C++11 compilers which are quite a bit<br>
smarter. It may work now (or not).<br>
<br>
If *that* can be solved then refactoring HttpHeaderFieldAttrs to a<br>
template is better way forward. Maybe followed by replacing or<br>
refactoring the HttpHeaderFieldInfo bits to avoid the performance<br>
problem you identified.<br></blockquote></div><br></div><div class="gmail_extra">Hi,<br></div><div class="gmail_extra">  no rambling, it's ok.<br></div><div class="gmail_extra">Actually, the performance improvement is just a nice byproduct, that's not the objective at all, as I tried explaining in the merge proposal.<br></div><div class="gmail_extra">The actual problem I was addressing is the multiple C casts used to try and coerce anything resembling a map<const char *, int> into an http/1 header table, simply because there is code in squid which implements that map as a header table (and rather naively, at that - linear scan is SO fifties ;) ).<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">In fact, I argue that the proposed version is actually simpler than the previous code; it can maybe even be simplified further but not much further, let's go over it a bit toghether if you want.<br><br></div><div class="gmail_extra">struct HttpDigestFieldAttrs is there simply to have a convenient representation of the header table DigestAttrs. It makes no attempt to be anything different. Can it be turned into GenericInitializerFromStringTo<enum foo> ? Yes. Is it worth it for 3 LOC? not so sure.<br><br></div><div class="gmail_extra">DigestFieldslookupTable_t is probably what you are lashing out at, I bet.<br>Yes, it could be made generic and templatized to represent a SBuf-to-anything with a static table initializer. I haven't tried that as that may be PMO and it may preclude turning the index from a map to an unordered_map with a table-specific hasher (gperf-generated or hand-written, it's rather trivial to do it in this and in many other cases we care about).<br>Doing that with a generic would be harder, require roughly about as much code, and for a <20-LOC class, would probably be overdesign.<br><br></div><div class="gmail_extra">The patch has been build-tested successfully on all platforms we care about, and run-tested (refactoring went in two phases: add new code, assert() that it behaves like the old code and run-test, rip old code out).<br><br></div><div class="gmail_extra">IMVHO HttpHeaderFieldAttrs is poor legacy, and deserves a thankful and merciful eviction from our code-base :)<br></div><div class="gmail_extra"><br></div><div class="gmail_extra"></div><div class="gmail_extra"><br>-- <br><div class="gmail_signature">    Kinkie</div>
</div></div>