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

[Kinkie]

>
> >> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150821/f9cfe52a/attachment-0001.html>