Sbuf review at r9331

Hi Kinkie,

    Here are a few Buffer comments based on r9331 of
lp:~kinkie/squid/stringng
The review is incomplete as there is too much in the way to do a full
high-level and low-level review. Some of the comments below will help
to clean up the way for a full review. The common theme is:

- Simplify to the bare minimum. We will optimize and add bells and
whistles later.
- Mimic standard interfaces and double check that you mimic them correctly.

Specific comments:

* Sbuf.h does not use *Exception classes. Most Buffer users would not
use them either. Move them and their prerequisites to
SbufExceptions.{cc,h} or something like that. As you know, I doubt most
of those exception classes are really needed/useful.

* class SBufStats {
    friend class SBuf;
    protected:

Make members public, remove friendship.

* Remove all *Stats::get() methods. C++ classes have copy constructors.

* The following comment about registration is a lie.
SBufStats(); ///constructor. Also registers with CacheManager

Note that it would probably be a bad idea to register with CacheManager
from the default constructor.

* Either all *Stats classes should have a constructor that initializes
its members or none.

* Move SbufStore outside of SBuf. You are not buying any extra security
or protection by embedding it, but making the code more complex and less
reusable.

* s/memalloc/memAlloc/ s/memblock/memBlock/ etc.

No need to add a third (fourth?) naming scheme. It is bad enough that we
have to mix size_type and canGrow styles.

* SBufStore(const SBuf &S);

SBufStore must not know about SBuf.

* SBufStore copy constructor and assignment operator are still not
declared correctly. Please please please review your code for const
correctness, once and for all.

* How about simply not implementing the not-to-be-used methods?
     * copy-contstructor. Not to be used.
     * \throw TextException always

If there is no implementation, the compiler and not the end-user will
complain if the method is actually used.

* Why is Sbuf MemPooled?! We are not supposed to allocate SBuf
dynamically, right? Did you mean to pool SbufStore instead?

* isNull is still public

* If you have introduced size_type, use it. Do not use size_t instead
except for size_type typedef.

* I doubt clear() should free the storage by default. Usually, we clear
to fill again. You even do that yourself in SBuf::assign(). Deallocating
and allocating would cost more than doing nothing. Let's not deallocate
by default, for now.

* Once again, please remove all exception specifications (I think you
have only empty throw()s left). For rationale, see my earlier reviews or
Item 13 "A pragmatic look at exception specifications" in Sutter's
"Exceptional C++ Style". Most if it should also be available at
http://www.gotw.ca/publications/mill22.htm

* See my earlier StringNg comments regarding truncateSelf, chopSelf,
import*, export*, substrSelf, and headUntil* naming and existence. Trim
them down to basics, use standard names, and remove "Self".

* remove dumpStats. Just give access to Stats object(s) that can dump.

* Why is cow() public?

* s/preAlloc/reserve/

* s/memhandle/store_/
And check other members for naming style. Use trailing underscores and
mixedCase consistently, everywhere.
 
* s/alloc_strategy/calcCapacity/ or similar. The method does not
allocate, change, or return any "strategy".

* Initialize Sbuf members in the constructor consistently. For example,
nreallocs is _sometimes_ initialized inside the constructor body. If
there are no performance concerns, initialize before the body and set
later if needed.

* Check that assignment operator sets everything. I think nreallocs was
forgotten.

* Parameter should be a const reference:
SBuf& SBuf::append(const SBuf S)

* There are too many similar-but-different append() implementations.
Can't you have one or two and have others call it with the right parameters?

* The following affects operator semantics and, hence, should be illegal
based on our last agreement regarding isNull:

    if (isNull() || S.isNull()) {
        debugs(SBUF_DEBUGSECTION,9,"\tone null");
        stats.qget++;
        return false;
    }

Not to mention that it feels wrong because it is asymmetric.

* Please change your substr() semantics (as implemented) to match that
of std::string when "from" is too big.

* I do not understand why you cannot keep the experted message without
duplicating it in the code below. Can you explain, please?

    msg=b.exportCopy();
    message=xstrdup(msg); //double-copy to avoid mismatching malloc and new
    delete msg;

* No need to check for NULL before delete in
    if (mem != NULL) {
        delete mem; //might have been freed by unref
        mem=NULL;
    }

You are duplicating the code in the common case.

* Declare when needed, for example inside for():
    size_type j;
    for (j=0;j<len_;j++) {

* I think you should merge realloc and preAlloc. I believe there are
already calls to realloc() where you should have used preAlloc().

* It is rather difficult to review the code for low-level correctness
with so many isNull exceptions. You need to find a way to push all
checks as deep as possible and make the higher level code more robust.
Some of the above changes will help with that.

* Your append(std::string) reverses the arguments of
std::string::append(string), which many developers would find extremely
confusing, especially since those arguments have defaults. Please fix.
Please double check other standard methods for similar problems.

HTH,

Alex.
Received on Sun Feb 01 2009 - 20:08:54 MST