<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 20, 2015 at 8:46 AM, 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"><div><div>On 16/08/2015 7:09 p.m., Kinkie wrote:<br>
> Hi all,<br>
> the attached patch does:<br>
> - EnumIterator, which now uses autoconf to detect std::underlying_type<br>
> - Removes header masks in HttpHeader.cc and HttpReply in favor of..<br>
> - gperf-generated header table with flags describing headers all collected<br>
> in one place<br>
> - replaces HttpHeader::getEntry with explicit vector iteration<br>
> - Implements and uses Http::HeaderTable.lookup(string-or-Http::HdrType) to<br>
> look up headers in the gperf-generated hash<br>
> - small optimizationi in HttpHeader::getLastEntry thanks to reverse<br>
> iterators<br>
> - use erase-remove pattern in HttpHeader::compact in place of resize()<br>
> - use std:: algorithms instead of explicit loop in HttpHeader::delById<br>
> - gets rid of httpHeaderCalcMask - masks are still used elsewhere but<br>
> generating them is now easier thanks to WholeEnum<type><br>
> - some readability improvements<br>
> - refactor HeaderManglers::track to avoid overloading of meaning the<br>
> enumEnd_ enumerator - please help me checking carefully I haven't altered<br>
> functionality here<br>
> - implements a case-insensitve hash function for SBuf<br>
> - all known headers are now unconditionally parsed. May not be acted upon<br>
> if some functions are not compiled in<br>
> - LookupTable now uses an unordered_map instead of map for backing storage.<br>
> No user-visible change for SBuf and std::string users unless they wish to<br>
> have a custom hasher<br>
><br>
> Most code has been run-tested (most notable exception: HeaderManglers),<br>
> farm-build-, coadvisor- and polygraph- tested. The main focus of this<br>
> refactoring is correctness and readability, c++11-ification and recovering<br>
> from the performance regression that was introduced ~10 days ago.<br>
><br>
<br>
<br>
</div></div>Here goes. Audit:<br>
<br>
<br>
* IMO the HeaderManglers really do need at least basic testing with<br>
these changes. A regression there affects widely used features.<br>
<br>
basic tests are easy enough. A simple proxy altering the Server header<br>
to a fixed value, adding some custom new header, and stripping something<br>
common like Expires should be sufficient to trigger any code problems<br>
and eyeball any major issues from the squidclient output to verify working.<br>
<br></blockquote><div><br></div><div>After some careful checking:<br></div><div>*_header_access and *_header_add work fine.<br></div><div>*_header_replace is broken, but it's been for a long time (I run-tested with code from february).<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
in acinclude/* and <a href="http://configure.ac" rel="noreferrer" target="_blank">configure.ac</a>:<br>
<br>
* tests and macro defines for C++11/0x back-compat are in<br>
acinclude/ax_cxx_0x_types.m4<br></blockquote><div><br></div><div>The file name seems inappropriate, but sure.<br></div><div>Done.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
in src/HttpHeader.cc:<br>
<br>
* please use {} on the for in HttpHeader::append().<br>
- aids readability of multiple nesting layers<br>
- same occurs elsewhere, I cant see the method name.<br></blockquote><div><br></div><div>findEntry, findLastEntry, refreshMask, getList. Done.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* "assert(! Http::HeaderLookupTable.lookup" looks - odd.<br>
- the extra whitespace does not really add anything and detracts from<br>
searching for "!Http::HeaderLookupTable.lookup"<br></blockquote><div><br></div><div>Changed.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* inconsistent changes ofr NULL->nullptr<br>
- eg in HttpHeader::findLastEntry() NULL is not changed, but elsewhere<br>
it usually is.<br></blockquote><div><br></div><div>That's actually intentional: I changed that only when something in the vicinity changed so not to make the patch any more enormous. I can blanket change if that's preferred.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
in src/HttpHeaderTools.cc:<br>
<br>
* HeaderManglers::dumpReplacement() has some odd line wrapping in the<br>
parameter lists of header_mangler_dump_replacement()<br></blockquote><div><br></div><div>Removed all line wrapping.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
in src/Makefile.am:<br>
<br>
* for testEnumIterator please add $(SQUID_CPPUNIT_LA) to the *_LDADD<br>
list instead of a *_DEPENDENCIES entry<br></blockquote><div><br></div><div>hm.. it's applied inconsistently. Documentation (at line 957) says to have it in both places.<br></div><div>testUfs, testRock : do not have it as a dependency<br></div><div>testHttpReply, testBoilerplate, testCacheManager, testDiskIO, testEvent, testEventLoop, test_http_range, testHttp1Parser, testHttpRequest, testStore, testString, testUrl, testSBuf, testSBufList, testConfigParser, testStatHist (and so on..) have it as a dependency<br>testACLMaxUserIP : used to have it as a dependency (but now it's commented out)<br></div><div> <br></div><div>For now I'm following documentation and leaving it in both places; if you have an opinion about how it should be I'll gladly carpet-apply that to the whole file.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* also please place tests/stub_* and base/EnumIterator.h in the nodist_*<br>
list from now on.<br></blockquote><div><br></div><div>Ok. Changed documentation at about line 951 to reflect that.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- the test _SOURCE list should now only contain the tests/test*.{h,cc}<br>
actually containing the unit-test code.<br></blockquote><div>Does this also apply to the files actually being tested? (X.h, X.cc)<br></div><div>I am fine either way, I would just like it to be reflected in the documentation. Not changed there for now, just in the testEnumIterator part.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- this helps prevent unit tests being the only reason files are dist<br>
(found a few like that already), so we can ensure they are linked to<br>
Squid main binary or libraries properly for automake dependency detection.<br>
<br>
<br>
in src/SBufAlgos.cc:<br>
<br>
* whats cctype needed for?<br></blockquote><div><br></div><div>tolower(3).<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
in src/SBufAlgos.h:<br>
<br>
* please doxygen comment new class CaseInsensitiveSBufHash<br></blockquote><div><br></div><div>Done.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
in src/adaptation/icap/ModXact.cc:<br>
<br>
* why is #if USE_ADAPTATION wrappers being added?<br>
- this entire file and even <a href="http://libicap.la" rel="noreferrer" target="_blank">libicap.la</a> should never be linked if<br>
adaptation is disabled.<br></blockquote><div><br></div><div>By mistake. Thanks for catching this.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
in src/base/EnumIterator.h:<br>
<br>
* please place the #if wrappers underneath the copyright blurb.<br>
- ensures the squid copyright is part of any code using that squid<br>
header, and<br>
- makes the maintenance tools detect it as ours not a third-party<br>
copyright.<br>
- same for tests/testEnumIterator.h<br></blockquote><div><br></div><div>Ok.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
* please doxygen comment new class EnumIteratorBase, ReverseEnumIterator,<br>
- eg, do these cover all values?<br>
where does it stop iterating?<br>
what value is produced for out-of-bounds ?<br>
uni or bi-directional ?<br></blockquote><div><br></div><div>Done. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
* for EnumRangeT and WholeEnum<br>
- utilize 'empty' lines between paragraphs<br>
- \note for highlighting additional notes<br>
- highlight example code with \code ... \endcode).<br></blockquote><div><br></div><div>Done <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
* s/Enum/EnumType/<br>
- to avoid any confusion with keyword 'enum'<br></blockquote><div><br></div><div>Done.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
in src/base/Makefile.am:<br>
<br>
* alphabetical order.<br></blockquote><div><br></div><div>Done. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
in src/http.cc:<br>
<br>
* please mark use of name.c_str() with "XXX: performance regression,<br>
c_str() reallocates"<br></blockquote><div><br></div><div>Actually, no in this case it doesn't unless ilen is exactly equal to one of the MemBlob MemPool sizes (possible but unlikely, I estimate in the 0.1% range).<br>There are some performance deltas but they are IMO quite marginal:<br></div><div>slowdowns: SBuf housekeeping, temp creation (of ilen+1 bytes) done in two steps instead of one<br></div><div>speedups: SBuf uses mempools, "*" case is checked before lowering case<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- the xmalloc removal is countered by the data-copy into "SBuf name".<br>
Making this a new second data-copy.<br>
<br>
<br>
in src/http/RegisteredHeaders.h:<br>
<br>
* please lock ACCEPT in as first entry;<br>
"<br>
enumBegin_ = 0,<br>
ACCEPT = enumBegin_,<br>
...<br>
"<br></blockquote><div><br></div><div>It already was, by assigning both enumBegin and ACCEPT value 0. Changed for clarity's sake.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* please delimit the methods and members of HeaderTableRecord with an<br>
empty line (and maybe another "public:")<br></blockquote><div><br></div><div>Done. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* a formatter run is going to be needed. Things like enum HdrKind<br>
contain tab-indented lines and double-empty lines.<br></blockquote><div><br></div><div>Yes.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
* HeaderLookupTable_t<br>
- need empty lines between paragraphs in doxygen comments again.<br>
- ie before the paragraph starting "Actual records are ..."<br>
<br>
* "///look record ..." --> "/// look ..."<br>
<br></blockquote><div>Ok<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
in src/http/RegisteredHeadersHash.cci:<br>
<br>
* is there any way to add a comment blurb "/* AUTO-GENERATED DO NOT EDIT<br>
*/" to the top of this file ?<br></blockquote><div><br></div><div>It can and I did, but it's not possible at the very top unfortunately.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
* also need to add the copyright boilerplate<br></blockquote><div><br>Same thing.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
in src/http/RegisteredHeadersHash.gperf:<br>
<br>
* need a way to add the copyright boilerplate as a gperf-ignored comment<br></blockquote><div><br></div><div>Done that. There is a preamble section into which I copied the C++ comments above.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
in src/tests/testEnumIterator.cc:<br>
<br>
* include path "testEnumIterator.h" -> tests/testEnumIterator.h<br></blockquote><div><br></div><div>Done.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* test enums should also use the " first = enumBegin_, " trick<br></blockquote><div><br></div><div>Done.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* testBidirectionalIter() also needs to test for reaching out-of-bounds<br>
conditions in either direction.<br>
- can it go fully across begin() -> end() -> begin() successfully in<br>
only the expected count of iterations? (no more, no less)<br></blockquote><div><br></div><div>Ok, added and tested.<br></div><br></div>Updated patch attached.<br></div><div class="gmail_extra"><br clear="all"><br>-- <br><div> Francesco</div>
</div></div>