<div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
>> in src/Makefile.am:<br>
>><br>
>> * for testEnumIterator please add $(SQUID_CPPUNIT_LA) to the *_LDADD<br>
>> list instead of a *_DEPENDENCIES entry<br>
>><br>
><br>
> hm.. it's applied inconsistently. Documentation (at line 957) says to have<br>
> it in both places.<br>
> testUfs, testRock : do not have it as a dependency<br>
> testHttpReply, testBoilerplate, testCacheManager, testDiskIO, testEvent,<br>
> testEventLoop, test_http_range, testHttp1Parser, testHttpRequest,<br>
> testStore, testString, testUrl, testSBuf, testSBufList, testConfigParser,<br>
> testStatHist (and so on..) have it as a dependency<br>
> testACLMaxUserIP : used to have it as a dependency (but now it's commented<br>
> out)<br>
><br>
> For now I'm following documentation and leaving it in both places; if you<br>
> have an opinion about how it should be I'll gladly carpet-apply that to the<br>
> whole file.<br>
<br>
<br>
</span>Documentation is outdated. I'm trying to work on _DEPENDENCIES removal<br>
now for automake 1.15 support as another incremental fix-up polishing as<br>
we add/change tests. Each test that has one does not check all its<br>
_LDADD indirect dependencies properly, so can inconsistently link tests.<br>
<br>
As far as SQUID_CPPUNIT_LA is concerned the issue seems to have been a<br>
bug that was resolved in older automake and/or cppunit versions. It<br>
works fine with just one _LDADD entry on the tests I've converted so far.<br>
<br>
<br>
It looks like I'm going to have to do a sorting sweep on *_LDADD anyway.<br>
So up to you.<br></blockquote><div><br></div><div>I guess it's best left as a cohesive effort, so I'll leave as-is for now. This patch is huge enough.<br></div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
><br>
>> * also please place tests/stub_* and base/EnumIterator.h in the nodist_*<br>
>> list from now on.<br>
>><br>
><br>
> Ok. Changed documentation at about line 951 to reflect that.<br>
><br>
><br>
>>  - the test _SOURCE list should now only contain the tests/test*.{h,cc}<br>
>> actually containing the unit-test code.<br>
>><br>
> Does this also apply to the files actually being tested? (X.h, X.cc)<br>
> I am fine either way, I would just like it to be reflected in the<br>
> documentation. Not changed there for now, just in the testEnumIterator part.<br>
><br>
<br>
</span>Yes. Especially them. The purpose is to ensure they are correctly listed<br>
in a _SOURCES for one of the libraries or binaries in normal builds.<br>
<br>
Sorry, I got a bit distracted on the _DEPENDENCIES removal work. Docs<br>
update still coming.<br></blockquote><div><br></div><div>Same answer as above then. I guess we'll have to have a "improve Makefile.am" effort.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="">>>  - 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>
>><br>
><br>
> tolower(3).<br>
><br>
<br>
</span>Ah. Thought so. Please use the xtolower() provided by libcompat.<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">
<span class="">>> in src/http.cc:<br>
>><br>
>> * please mark use of name.c_str() with "XXX: performance regression,<br>
>> c_str() reallocates"<br>
>><br>
><br>
> Actually, no in this case it doesn't unless ilen is exactly equal to one of<br>
> the MemBlob MemPool sizes (possible but unlikely, I estimate in the 0.1%<br>
> range).<br>
> There are some performance deltas but they are IMO quite marginal:<br>
> slowdowns: SBuf housekeeping, temp creation (of ilen+1 bytes) done in two<br>
> steps instead of one<br>
> speedups: SBuf uses mempools, "*" case is checked before lowering case<br>
><br>
<br>
</span>Okay. If you are sure. I have thought that a few times then found SBuf<br>
reallocating based on seemingly just the lock counter.<br></blockquote><div><br></div><div>if sbuf has single owner, c_str just makes sure that after the end of the valid area there is a \0. That may require re-allocing if end of valid area is at end of owned memory space.<br></div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
>><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>
>><br>
><br>
> It can and I did, but it's not possible at the very top unfortunately.<br>
><br>
>><br>
>> * also need to add the copyright boilerplate<br>
>><br>
><br>
> Same thing.<br>
><br>
<br>
</span>Fine. At least they there now to silence the "CHECK COPYRIGHT"<br>
maintenance warnings. I will see what else is needed by the scripts once<br>
this is all merged.<br>
<br>
<br>
+1. with the below documentation polish :-)<br>
<br>
New bits:<br>
<br>
in src/HttpHeader.cc<br>
* HttpHeader::append now s/addEntry (/addEntry(/<br>
<br>
<br>
in src/HttpReply.cc:<br>
* one more for-loop needing brackets<br></blockquote><div><br></div><div>two, actually. Both done.<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/SBufAlgos.h:<br>
* please use doxygen \code ... \endcode for code exammples.<br>
 - definitely NOT using shell-eval quotes<br></blockquote><div><br></div><div>Ok<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/acl/HttpHeaderData.h:<br>
* please re-aligning the comments after code shrinkage :-)<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/EnumIterator.h:<br>
<br>
* one empty line after the boilerplate please.<br>
 - surprised the formatting run did not catch that, it does on master.<br>
 - same in 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>
* s/or c++11 strongly-typed/or C++11 strongly-typed/<br>
 - "C++11" is a noun/acronym.<br></blockquote><div><br></div><div>Ok<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* please do not quote around begin()/end()/rbegin()/rend() names<br>
 - doxygen should highlight and link naturally<br>
 - specialy not with shell-eval quotes.<br>
 - might be best to look for more of those in this patch if you can. I<br>
cant because my command-line tries to execute something just looking for<br>
a string involving them.<br></blockquote><div><br></div><div>Removed all in EnumIterator.h, I don't think there's any more.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* s/doesn't/does not/ and s/you'll/you will/<br>
 - documentation is formal language.<br></blockquote><div><br></div><div>Ok<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* all this "prefix increment" / "postfix increment" operator documentation<br>
 - if its worth writing them at all use doxygen "///".<br></blockquote><div><br></div><div>Removing.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* EnumRangeT<br>
 - s/mote/note/ ?<br>
 - and shell-eval quotes again<br></blockquote><div><br></div><div>Yes; fixed.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* WholeEnum<br>
 - doxygen syntax is:  /** [brief] \n ... \n */<br>
 - if you start a multi-line description on the first line after "/**"<br>
things get weird by only using part of the text in some output douments.<br>
 - so keep 1-liners for brief description, and omit anything on the same<br>
line as "/**" if you need just need a good long description.<br>
 - this is why I generally avoid the 1-liner brief prefixes.<br></blockquote><div><br></div><div>Shortened. I've also taken the chance to expand the use examples to be more understandable.<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/esi/Libxml2Parser.h<br>
* revert to trunk version.<br></blockquote><div><br></div><div>Ok.<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/http/RegisteredHeaders.h:<br>
* HeaderTableRecord if its worth using '//' on the bools its worth using<br>
'///< '<br></blockquote><div><br></div><div>Kept documentation<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>
 - s/header types' definitions/header definitions/<br>
 - on BadHdr; use doxygen '///< '<br></blockquote><div><br>Ok, although it's kind of internal.<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/tests/testEnumIterator.cc<br>
* in testBidirectionalIter and testUnsignedEnum<br>
 - how about using &&count<=20 to terminate the loops if things go very<br>
wrong?<br></blockquote><div><br></div><div>Done by explicit test in the loop in order to allow using a specific comment<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
 - I'd hope for something similar in the testRangeFor*() loops, but not<br>
sure if/how its possible to enforce an exit with range-for being<br>
hyper-threaded sometimes.<br></blockquote><br clear="all"></div>Up to the compiler to enforce. Would probably disable parallelization, but who cares: it must be transparent to us anyway.<br><br></div><div class="gmail_extra">Committing, testing and merging.<br></div><div class="gmail_extra">Thanks!<br></div><div class="gmail_extra"><br>-- <br><div class="gmail_signature">    Francesco</div>
</div></div>