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 _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
