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

> 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