Re: SBuf review at r9370

Alex Rousskov wrote:
> 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.

Aye. we have util.h and libmisc for generic utility functions like this.
(even though that may change later/sooner its worth making them outside
string stuff to start with).

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

As long as MemBlob::grow() returns a pointer/reference to larger self or
larger copy that SBuf can change to use.
AND as long as SBuf::grow() is only needed internally. Yet another
wrapper indirection is the last thing we need for String.

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

max() and min() are themselves provided in the compat layer right next
to "#define XMAX(a,b) (max(a,b))" and XMIN likewise.

Does anyone know of a reason why we can't simply use min() and max()
directly?

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

FYI: I've been using /*...*/ above the #include to mark headers which
need to be removed ASAP but can't for some reason as commented.
Sorry if this style has confused anyone.
(not to be confused with the _new_ comments Alex is speaking of)

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

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE6 or 3.0.STABLE13
   Current Beta Squid 3.1.0.5