SBuf review at r9370

Hi Kinkie,

    Here are a few corrections for
http://bazaar.launchpad.net/~kinkie/squid/stringng at r9370.

* Remove isLiteral and the corresponding code/logic. We might add this
very minor performance optimization as a non-public Blob member once the
code is known to work well. For now, the code is confusing enough
without it.

Once isLiteral is removed, compare with the prototype pointer instead of
testing isLiteral in cow().

* Remove isImported. Copy and then free the buffer when importing
instead. Same motivation as in the isLiteral comment above.

* SBuf copy constructor and assignment operator still initialize members
differently.

* Some throw() declarations are still left in the code. Grep for them.

* cow() logic feels wrong because grow() does not just grow(). Grow is
discussed below. The cow() code should be something very simple like:

    if (we are alone)
        return false; // no need to copy

    store_ = new MemBuf(*store_); // get ourselves an exclusive copy of
the blob
    return false;

No grow() magic -- we are getting an exclusive copy, not growing something.

* roundto(5,5) should return 5 but returns 10. The comment and
implementation should be fixed. The method probably does not belong here
though.

* estimateCapacity() is too complex to understand its performance
effects, especially since nreallocs is a very strange animal. I suggest
that we keep it very simple and add optimizations later, if needed.
Something like
   
    granularity = desired < PageSize ? 16 : PageSize;
    return roundto(desired, granularity)

is much easier to comprehend, analyze, and optimize. Let's postpone
complex optimizations.

* To me, compare() returns the opposite of what strcmp() does. "This"
should be treated as the first parameter of strcmp() and not the second
one, IMO.

* startsWith() is buggy -- it ignores the contents of its parameter's
buffer.

* SBuf::consume() returns a copy of SBuf. It should return a reference
to this or void.

* Move "if !canAppend then grow" logic to a single method. Call it
reserve(). More on grow() below.

* grow() comments are wrong. The method does not always allocate a new
buffer.

* grow() implementation is odd. It ignores the remaining blob space
size. Am I missing something? I have a feeling you implemented something
other than grow() so that you can rely on its strange combination of
do-not-grow and grow-even-if-you-do-not-have-to logic in the rest of the
code. This needs to be fixed in grow() and in the callers().

* I wonder if SBuf::grow() should be moved to MemBlob that may
(eventually) have a low-level knowledge on how to do that efficiently.
What do you and others think?

* Please use .length() everywhere you can. Do not mix .len_ and .length().

* s/shortcut: same length and same store/shortcut: same store, offset,
and length/

* SBuf::consume() comments are stale, talking about NULL.

* "actual" in SBuf::consume() should be const.

* getStats should return a const reference not a copy of, potentially
large, stats object.

* Please use XMIN or XMAX instead of manually comparing values with a
?-operator. It is much easier to see and check the logic that way.

* Exceptions thrown by SBuf will lack source code location info, right?
I think we need to add a ThrowHere(ex) macro that supplies that to the
ex parameter.

* bufEnd() should return the pointer to the byte after the last one. You
can call it end(). If you find yourself accessing the last byte often,
add last() that returns a char.

* s/prototype/starter/ or s/prototype/primer/
And no need to prefix the corresponding member with store, IMO.

* s/A SBuf is GUARRANTEED to be defined.//
Any C++ object is "defined".

* cow() declaration comments are stale

* Please rename substrSelf() as discussed.

* Please capitalize static members.

* s/roundto/RoundTo/

* s/importCString/absorb/ or s/importCString/absorbCString/
because you are taking full control over the string memory.

* roundto is declared as a global static function in a .cci file. It
should probably be declared as an inline.

* Use static_cast in SBuf::plength() and elsewhere. It would be much
easier to find and tune casting later if needed.

* s/store/blob/ or s/store/mem/
We have enough store* stuff in squid.

* exportCopy comments are wrong or stale. It does not return NULL. It
should throw on memory allocation errors (but probably does not yet).
The comment should say that the returned string is zero terminated.

* Remove comments after #includes. Some tools break on C++ comments
there and you are partially lying and partially not supplying any new
info there.

    #include "config.h" //various typedefs
    #include "Debug.h" //debugs etc

At this point, I got "Launchpad is offline for scheduled maintenance. We
should be back soon." error so I will have to look at MemBlob later.

HTH,

Alex.
Received on Thu Feb 26 2009 - 00:46:42 MST