Re: Sbuf review at r9331

Further improvements:

> * SBufStore(const SBuf &S);
>
> SBufStore must not know about SBuf.

Done. It's now a first-class citizen, without any SBuf knowledge. In
the process I've removed some methods and migrated others to
char*-based.

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

You're right. SBufStore and its smaller storage sizes should be pooled.

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

Ok. Used it everywhere.

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

I've commented the line releasing the storage. Worst-case, it will
just realloc lazily which is still ok.

> * remove dumpStats. Just give access to Stats object(s) that can dump.
Done. Now both SBuf and SBufStore have const getters instead.

> * Why is cow() public?

Now private, as discussed.

> And check other members for naming style. Use trailing underscores and
> mixedCase consistently, everywhere.
Done. I hope I didn't miss anything, will recheck.

> * Check that assignment operator sets everything. I think nreallocs was
> forgotten.
It was intentional as discussed, but I've now added also that to the
assignment operation.
It's an optimization anyways so its usefulness is debatable until we
get some hard numbers..

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

Fixed. Even though I'll admit I don't like the innards of xstrndup
much so I had to basically reimplement it in client-code (there's a
strlen() too much in xstrndup).

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

Done. Now it only performs a (then-unimplemented) isLiteral check.

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

Do you mean that this should read
for (size_type 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().

Done. The merged call is now called grow().

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

I have now reduced the number of (constructor/assign/append) functions
by one by grouping the char*-based ones and using default arguments.
The whole thing now looks much more polished and consistent.

Next steps, mempooling SBufStore and its backing storage.

-- 
    /kinkie