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.