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