[squid-dev] [PATCH] Coverity fixes, part 3

[Amos Jeffries]

On 27/07/2015 4:48 a.m., Kinkie wrote:
> HI,
>   the low-hanging fruits from Coverity's analysis have been picked, now
> working on somewhat more complex fixes.
> The attached patch takes a hint from two benign coverity defects to:

> - refactor Digest auth's field lookup table to use a std::map instead of
> abusing httpHeaderFieldsInfo; this improves readability and size of the
> code, and has the added small bonus of lowering the lookup cost from linear
> to logarithmic
> 
> the Digest code has been run-tested. I'd like feedback on its style, as
> httpHeaderFieldsInfo is abused similarly elswehere and I'm considering to
> apply it elsewhere as well; it can then be further refined to get O(1) via
> a carefully-chosen hash (via std::unsorted_map)
> 

My thoughts: (sorry if its a bit rambling)

I dont like spawning lots of new classes for basic things. I know its
essentially the C++ way. But applying pattern theory can go too far
sometimes. And this patterns usage is one case where I can see exactly
that happening.

You have a struct, a class, an enum and array. Thats a lot of custom
infrastructure just to represent a simple set of name:id. We could as
easily have std::map<const char*, enum> holding that directly and avoid
all the local types except enum.

There are already 5 headers currently in Squid that need these same
operations applied, and at least as many more that should but dont even
use the current HttpHeaderFieldAttrs related types yet.



struct HttpDigestFieldAttrs should be a template class so we dont have
to re-implement a new little struct for each enum.

BUT, notice that generalizing that same structure is also where the enum
casting hack for HttpHeaderFieldAttrs comes from in the first place.
 The probkem was template style breaks with C++03 compilers thinking all
enums are of "int" type. Enter lots of repeated-instantiation complaints
from dumb compilers.
 I've not tried it with current C++11 compilers which are quite a bit
smarter. It may work now (or not).

If *that* can be solved then refactoring HttpHeaderFieldAttrs to a
template is better way forward. Maybe followed by replacing or
refactoring the HttpHeaderFieldInfo bits to avoid the performance
problem you identified.

Amos