Hi, I forgot to mention another couple of changes: - I added an explicit copy-constructor to import std::string - I relaxed the behavior of the chop() method for out-of-bounds cases: instead of throwing, it'll now ignore the part of the specified limits that is out of bounds.
On Mon, Apr 1, 2013 at 7:43 PM, Kinkie <gkin...@gmail.com> wrote: >> My audit, I'm sure Alex will have more. >> >> +1 for the src/MemBlob.cc and src/MemBlob.h operator addition to go into >> trunk immediately. The rest has a lot of polishing still needed. > > Ok, I'll wait for Alex' review before merging. > >> Design Question: with all the stats being collected is it worth collecting >> an assign-to-self counter? to see if there are any obvious logic bugs in >> usage which we could avoid? > > I tried to measure most operations clusters, to measure the > effectiveness of the shared-storage approach during the transition > phase. > Once we have a clearer picture, we can gradually remove the > meaningless statistics. > >> in src/SBuf.cc: >> >> * can we consider making it a guideline to place constructor lists with the >> colon after the constructor definition and the initializer list one to a >> line following, with indentation? It makes it so much clearer to see what >> member is being initialized when there is any kind of complex parameters for >> it. NP: astyle enforces only the indentation. >> ie: >> +SBuf::SBuf() : >> + store_(GetStorePrototype()), >> + off_(0), >> + len_(0) >> +{ > > It uses lots of vertical space, and unintialized members can get > caught by Coverity anyway. > This said, I have no strong opinion. > >> ** along with this can we ensure that the constructor arguments are all on >> one line. If it breaks the 80-character line style preference we need to >> reconsider what its doing. >> - case in point there being the constructor for OutOfBoundsException. > > Well, OutOfBoundsException is 19 chars long; add the class name and > half the vertical screen estate has been used. > Again, no strong opinion. I don't think we should put constraints on > length of class names etc. Besides, implementation classes are not > really meant to be read often are they? > >> * SBuf::vappendf() >> - comment "//times 1.2 for instance..." does nt match the reserveSpace() >> call it is about. The second line of comment can be erased. > > Done. > >> - according to the comments about Linux vsnprintf() return of -1 is >> guaranteed to violate the Must() condition when reserveSpace(-1*2) is >> called. The if-statement sz<0 condition ensures it. > > Changed the two cases, now throws TextException in case of output > error - I'd assume it's unrecoverable. > >> - please avoid C-style casting in new code whenever possible. May occur >> elsewhere also. > > Ok. > >> - the line "+ if (operator[](len_-1) == 0) {" should be comparing >> against '\0' instead of 0 to ensure byte/word alignment changes by the >> compiler don't involve magic type castings. > > Ok, done. > >> * SBuf::compare() please move the stats.compareSlow counting up top. Just >> under the Must() keeps it in line with the pther stats collections. > > Ok. > >> * SBuf::startsWith() lacks the length debugs() statement used in >> operatror==() immediately below it. > > Added. > >> >> * SBuf::copy() why not do: const SBuf::size_type toexport = min(n, >> length()); > > Done. > >> * SBuf::forceSize() - we need to consider what happens (or already has >> happened) when a size greater than capacity is forced. > > Right. > What about throwing an OutOfBoundsException? Overflow has likely happened. > Not changed yet. > >> * SBuf::chop() - the TODO optimization is not possible. We cannot be >> completely sure we are teh only user of the > > Removed the TODO. It's a very unlikely case anyway. > >> * SBuf::rfind() - typo in weridness comment "memrhr". > > Fixed. > >> * Please move operator <<() to an inline function. > > Done (via .cci for now. I recall Alex was suggesting to get rid of > .cci, will move to .h if I can find confirmation of that). > >> * SBuf::reAlloc() comment needs to be doxygen format. > > Done. > >> in SBuf.h >> >> * Debug.h is not needed here. Please move to SBuf.cci instead. >> - probably also SquidString.h can also be removed, but check that with a >> build test. > > Needed for String import/export functions. > Hopefully String will just go in a relatively short time after merging. > >> * several places saying "* \note bounds is 0 <= pos < length()" are >> incorrect. SBuf::checkAccessBounds() tests for: "0 <= pos <= length()" > > Then checkAccessBounds is wrong. Changed unit test accordingly. > >> * s/minCapcity/minCapacity/ > > Fixed. > >> * SBuf::operator []() documentation is incorrect. It *does* check access >> bounds, but returns \0 for any bytes outside instead of throwing exception. > > Gah! Changed to match documentation. > >> * SBuf::vappendf() documentation lacking empty line separate from earlier >> method declaration above. > > Added. > >> * SBuf::chop() >> - documentation contains HTML tags, Please avoid that. 'i' and 'n' (quoted) >> is a better written style of emphasis. > > Fixed. > >> - documentatin of n==0 case lacks the info that SBuf is cleared, like the >> pos>length() case does. > > Fixed. > >> * SBuf::rawContent() does not call cow(), so please call it a "read-only >> pointer" in the documentation. > > Done. > >> * SBuf::print() >> - one-liner doxygen uses ///. >> - it could document better what the method does to differentiate itself >> from dump() other than "print" which is obvious from the name. > > Clarified the documentation. > >> * SBuf::operator ==() has a useless doxygen comment. Please make it a normal >> code comment "boolean conditionsls" or just remove. We have no style >> guideline around this but the existing comment is simply useless. > > Done. > >> * please mark the SBuf::toString() converter as \deprecated. > > Done. > >> * SBuf::GetStats() doxygen one-liner. same elsewhere ie SBuf::length(). > > Done. > >> * SBuf::rawSpace() - the line "This call forces a cow()" duplicates and >> contradicts the above comments about COW *if necessary*. IMO it should say >> somethign like "guarantees the space is safe to write to". > > Now reads: " A COW will be performed if necessary so that the returned > pointer can be written to without unwanted side-effects". Is this > good? I'm trying to imply that this method is not really encouraged. > >> * SBuf::c_str() s/will remain valid if the caller keeps holding/will remain >> valid only if the caller keeps holding/ > > Ok. > >> * if you are going to make TODO comments doxygen ones, please use \todo tag >> instead of our developers "TODO:" > > Right. > >> * SBuf::ReserveSpace() >> "... >> + * least minSpace bytes of append-able backing store (on top of the >> + * currently-used portion). >> " >> - should probably say: "at least minSpace bytes of unused backing store >> following the currently used portion." > > Changed > >> >> * SBuf::find() both versions share a documentation problem >> - s/if startPos is SBuf::npos, npos is always returned/if startPos is >> SBuf::npos or greater than SBuf::length(), npos is always returned/ > > fixed > >> >> * SBuf::rfind() both versions share a documentation problem >> - s/if unspecified or npos, the whole SBuf is considered./if npos or >> greater than length(), the whole SBuf is considered./ > > It worse than that. > It should be: > if npos or greater than length(), the whole SBuf is considered. > If < 0, npos is always returned > >> in src/SBuf.cci >> >> * Sbuf::bufEnd() is lacking any checks about whether the space returned is >> safe to use, no cow() or whatever. >> - these need to be added or at least documented that they are not done and >> the caller is responsible. > > Clarified the documentation > >> - if the documentation is left unchaned, please use /// doxygen comment for >> one-liners. >> - s/if (aFileNamek != 0)/if (aFileName)/ or at least use !=NULL. > > used NULL > >> in src/OutOfBoundsException.h >> * please follow naming guideline "Do not start names with an underscore". > > Fixed. > >> in src/SBufExceptions.cc >> >> * OutOfBoundsException >> - please initialize OutOfBoundsException::_buf and >> +OutOfBoundsException::_pos in the constructor initializer list. > > Done. > >> - the note about pass-by-copy seems to be incorrect, was it about the _buf >> being a copy/reference ? > > Removed comment, it's useless. > >> in src/SBufStream.h >> >> * class SBufStreamBuf >> - class documentation -> doxygen one-liner. Or expand with a see-also >> reference, probably to std:: documentation source on what a streambuf is / >> does / requires. > > Done. > >> - please be consistent with use of doxygen for members comments. > > Non-doxygen comments are for nonpublic members. I've turned to doxygen > style, I hope they're clearer now. > >> in src/tests/testSbuf.cc >> >> * the XXX in testSBuf::testSBufConstructDestructAfterMemInit() has already >> been done and can be removed. > > Ok > >> * testSBuf::testRawSpace() is broken, >> - strcat() is appending to the end of whatever strng exists in the >> uninitialized buffer "rb". Logically that means a) any non-zero bytes will >> be left as prefix to "rb", and b) if there are any strcat() may >> buffer-overrun. > > Hm.. rb points to the end of the store used by s2, after a cow() and > with a guarantee of having more than strlen(fox2)+ 1 bytes of > appendable storage. The test is broken but not because rb is > uninitialized: it is, but it is null-terminated by chance so strcat > may buffer-overrun searching for the end of the appended-to c-string. > The I've corrected the problem by using strcpy instead of strcat. > >> - rawSpace() is used but forceSize() is not used to tell s2 that its >> content has been extended into that space > > yup. *shame*. The test was skipped, this is why it didn't fail. > Corrected, and debugged through it, it should be fine now. > >> --> AFAICT the expected outcome from all these is that we should still >> expect: s1 != s2 since the SBuf lengths differ and the rawSpace after S2 may >> contain random unknown contents. > > Yes. > >> * testSBuf::testChop() is missing tests for most of the border cases > > Added some manual tests and a simple autotester to match SBuf::substr > to std::string::substr. > SBuf::substr is implemented via chop, so it is implicitly tested. > >> *** run out of time to audit today. A quick review of the rest of the test >> units shows a similar lack of coverage on edge cases. Some we will want to >> write fuzzers for and others we will want to add explicit manual tests for. >> TODO on all that. >> >> PS. skipped the SBufFindTest code for now. > > Attached is a bundle containing the changes from this review. > Thanks! > > > -- > /kinkie -- /kinkie