[squid-dev] [PATCH] Coverity(-inspired) fixes part four, HttpHeader refactor

[Amos Jeffries]

On 16/08/2015 7:09 p.m., Kinkie wrote:
> Hi all,
>   the attached patch does:
> - EnumIterator, which now uses autoconf to detect std::underlying_type
> - Removes header masks in HttpHeader.cc and HttpReply in favor of..
> - gperf-generated header table with flags describing headers all collected
> in one place
> - replaces HttpHeader::getEntry with explicit vector iteration
> - Implements and uses Http::HeaderTable.lookup(string-or-Http::HdrType) to
> look up headers in the gperf-generated hash
> - small optimizationi in HttpHeader::getLastEntry thanks to reverse
> iterators
> - use erase-remove pattern in HttpHeader::compact in place of resize()
> - use std:: algorithms instead of explicit loop in HttpHeader::delById
> - gets rid of httpHeaderCalcMask - masks are still used elsewhere but
> generating them is now easier thanks to WholeEnum<type>
> - some readability improvements
> - refactor HeaderManglers::track to avoid overloading of meaning the
> enumEnd_ enumerator - please help me checking carefully I haven't altered
> functionality here
> - implements a case-insensitve hash function for SBuf
> - all known headers are now unconditionally parsed. May not be acted upon
> if some functions are not compiled in
> - LookupTable now uses an unordered_map instead of map for backing storage.
> No user-visible change for SBuf and std::string users unless they wish to
> have a custom hasher
> 
> Most code has been run-tested (most notable exception: HeaderManglers),
> farm-build-, coadvisor- and polygraph- tested. The main focus of this
> refactoring is correctness and readability, c++11-ification and recovering
> from the performance regression that was introduced ~10 days ago.
> 


Here goes. Audit:


* IMO the HeaderManglers really do need at least basic testing with
these changes. A regression there affects widely used features.

basic tests are easy enough. A simple proxy altering the Server header
to a fixed value, adding some custom new header, and stripping something
common like Expires should be sufficient to trigger any code problems
and eyeball any major issues from the squidclient output to verify working.


in acinclude/* and configure.ac:

* tests and macro defines for C++11/0x back-compat are in
acinclude/ax_cxx_0x_types.m4


in src/HttpHeader.cc:

* please use {} on the for in HttpHeader::append().
 - aids readability of multiple nesting layers
 - same occurs elsewhere, I cant see the method name.

* "assert(! Http::HeaderLookupTable.lookup" looks - odd.
 - the extra whitespace does not really add anything and detracts from
searching for "!Http::HeaderLookupTable.lookup"

* inconsistent changes ofr NULL->nullptr
 - eg in HttpHeader::findLastEntry() NULL is not changed, but elsewhere
it usually is.


in src/HttpHeaderTools.cc:

* HeaderManglers::dumpReplacement() has some odd line wrapping in the
parameter lists of header_mangler_dump_replacement()


in src/Makefile.am:

* for testEnumIterator please add $(SQUID_CPPUNIT_LA) to the *_LDADD
list instead of a *_DEPENDENCIES entry

* also please place tests/stub_* and base/EnumIterator.h in the nodist_*
list from now on.
 - the test _SOURCE list should now only contain the tests/test*.{h,cc}
actually containing the unit-test code.
 - this helps prevent unit tests being the only reason files are dist
(found a few like that already), so we can ensure they are linked to
Squid main binary or libraries properly for automake dependency detection.


in src/SBufAlgos.cc:

* whats cctype needed for?


in src/SBufAlgos.h:

 * please doxygen comment new class CaseInsensitiveSBufHash


in src/adaptation/icap/ModXact.cc:

* why is #if USE_ADAPTATION wrappers being added?
 - this entire file and even libicap.la should never be linked if
adaptation is disabled.


in src/base/EnumIterator.h:

* please place the #if wrappers underneath the copyright blurb.
 - ensures the squid copyright is part of any code using that squid
header, and
 - makes the maintenance tools detect it as ours not a third-party
copyright.
 - same for tests/testEnumIterator.h

* please doxygen comment new class EnumIteratorBase, ReverseEnumIterator,
 - eg, do these cover all values?
   where does it stop iterating?
   what value is produced for out-of-bounds ?
   uni or bi-directional ?

* for EnumRangeT and WholeEnum
 - utilize 'empty' lines between paragraphs
 - \note for highlighting additional notes
 - highlight example code with \code ... \endcode).

* s/Enum/EnumType/
 - to avoid any confusion with keyword 'enum'


in src/base/Makefile.am:

* alphabetical order.


in src/http.cc:

* please mark use of name.c_str() with "XXX: performance regression,
c_str() reallocates"
 - the xmalloc removal is countered by the data-copy into "SBuf name".
Making this a new second data-copy.


in src/http/RegisteredHeaders.h:

* please lock ACCEPT in as first entry;
"
  enumBegin_ = 0,
  ACCEPT = enumBegin_,
  ...
"

* please delimit the methods and members of HeaderTableRecord with an
empty line (and maybe another "public:")

* a formatter run is going to be needed. Things like enum HdrKind
contain tab-indented lines and double-empty lines.

* HeaderLookupTable_t
 - need empty lines between paragraphs in doxygen comments again.
 - ie before the paragraph starting "Actual records are ..."

* "///look record ..." --> "/// look ..."


in src/http/RegisteredHeadersHash.cci:

* is there any way to add a comment blurb "/* AUTO-GENERATED DO NOT EDIT
*/" to the top of this file ?

* also need to add the copyright boilerplate


in  src/http/RegisteredHeadersHash.gperf:

* need a way to add the copyright boilerplate as a gperf-ignored comment


in  src/tests/testEnumIterator.cc:

* include path "testEnumIterator.h" -> tests/testEnumIterator.h

* test enums should also use the " first = enumBegin_, " trick

* testBidirectionalIter() also needs to test for reaching out-of-bounds
conditions in either direction.
 - can it go fully across begin() -> end() -> begin() successfully in
only the expected count of iterations? (no more, no less)


Cheers
Amos