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

On 30/03/2013 7:05 a.m., Kinkie wrote:
> Hi,
> it was easier than expected. Here's a merge bundle; it intentionally
> forgets commit history, it can remain in the branch. This bundle
> passes a full testsuite build on my kubuntu quantal.
> It contains only what was audited so far:
> - SBuf
> - SBufExceptions
> And I think it was audited, but I'm not sure:
> - SBufStream
>
> Left out but ready for audit are:
> - cachemgr integration
> - tokenizer
>
> Should be ready but I'd like to doublecheck
> - SBufList (intended to replace StrList)
> - SBufUtils
>
> Thanks!
>

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.

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?

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)
+{

  ** 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.

* 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.
  - 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.
  - please avoid C-style casting in new code whenever possible. May
occur elsewhere also.
  - 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.

* SBuf::compare() please move the stats.compareSlow counting up top.
Just under the Must() keeps it in line with the pther stats collections.

* SBuf::startsWith() lacks the length debugs() statement used in
operatror==() immediately below it.

* SBuf::copy() why not do: const SBuf::size_type toexport = min(n,
length());

* SBuf::forceSize() - we need to consider what happens (or already has
happened) when a size greater than capacity is forced.

* SBuf::chop() - the TODO optimization is not possible. We cannot be
completely sure we are teh only user of the

* SBuf::rfind() - typo in weridness comment "memrhr".

* Please move operator <<() to an inline function.

* SBuf::reAlloc() comment needs to be doxygen format.

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.

* several places saying "* \note bounds is 0 <= pos < length()" are
incorrect. SBuf::checkAccessBounds() tests for: "0 <= pos <= length()"

* s/minCapcity/minCapacity/

* SBuf::operator []() documentation is incorrect. It *does* check access
bounds, but returns \0 for any bytes outside instead of throwing exception.

* SBuf::vappendf() documentation lacking empty line separate from
earlier method declaration above.

* SBuf::chop()
  - documentation contains HTML tags, Please avoid that. 'i' and 'n'
(quoted) is a better written style of emphasis.
  - documentatin of n==0 case lacks the info that SBuf is cleared, like
the pos>length() case does.

* SBuf::rawContent() does not call cow(), so please call it a "read-only
pointer" in the documentation.

* 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.

* 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.

* please mark the SBuf::toString() converter as \deprecated.

* SBuf::GetStats() doxygen one-liner. same elsewhere ie SBuf::length().

* 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".

* SBuf::c_str() s/will remain valid if the caller keeps holding/will
remain valid only if the caller keeps holding/

* if you are going to make TODO comments doxygen ones, please use \todo
tag instead of our developers "TODO:"

* 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."

* 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/

* 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./

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.
  - if the documentation is left unchaned, please use /// doxygen
comment for one-liners.
  - s/if (aFileName != 0)/if (aFileName)/ or at least use !=NULL.

in src/OutOfBoundsException.h
  * please follow naming guideline "Do not start names with an underscore".

in src/SBufExceptions.cc

* OutOfBoundsException
  - please initialize OutOfBoundsException::_buf and
+OutOfBoundsException::_pos in the constructor initializer list.
  - the note about pass-by-copy seems to be incorrect, was it about the
_buf being a copy/reference ?

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.
  - please be consistent with use of doxygen for members comments.

in src/tests/testSbuf.cc

* the XXX in testSBuf::testSBufConstructDestructAfterMemInit() has
already been done and can be removed.

* 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.
  - rawSpace() is used but forceSize() is not used to tell s2 that its
content has been extended into that space
  --> 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.

* testSBuf::testChop() is missing tests for most of the border cases

*** 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.

Amos
Received on Sat Mar 30 2013 - 08:32:21 MDT