Re: [RFC] Time to talk about StringNG merge again?

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 <gkinkie_at_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