Re: Sbuf review at r9331

On Sun, Feb 1, 2009 at 9:08 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> 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.

done.

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

Done.

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

Right. That's done in SBufExtras:SBufStatsCacheManagerAction constructor.

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

Yes. The comment is bogus and has been removed.

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

Added. For clarity if nothing else.

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

This can be addressed in three ways that I can think of:
1- leaving as-is
2- bringing that out, with private constructors and friendship to
select who can instantiate what.
3- bringing that out with big red signs in the class declaration not
to (ab)use it.

would 2. be acceptable to you?

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

Done, for more or less all members.

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

It knowing about SBuf is a serious design help wrt SBuf::consume()
If it didn't, we'd have to take a trip through char*.
If it can't be accepted, I'll do so, but if it's possible at all I'd
like to keep it.

> * 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.
> If there is no implementation, the compiler and not the end-user will
> complain if the method is actually used.

Ok. I had set off in that way but for some reason it didn't compile. Fixed.

> * \throw TextException always

I've tried to be more specific, all more-specific exceptions inherit
from TextException.
I'd like to understand what's the benefit in having only one exception kind.

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

SBuf is a fixed-size object. MemPooling avoids heap fragmentation, doesn't it?

> * isNull is still public

As of two days ago, it doesn't exist anymore.

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

I've only left it for those cases where we go to char* land.
I have no issues with changing it if you feel it's the best thing to do.

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

It is a possible optimization, but it's tricky and it involves
checking refcounts and slicing history of the SBufStore, and the same
practical result can be obtained by mempools (as it is now). In other
words, I'd leave it as-is.

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

Yes, I had left the empty ones since a previous comment of yours told
that they may be non-useless.
I've now removed them all.

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

They're now all gone except for substr and substrSelf. The dualism is
needed because the former creates a new String and the latter modifies
in-place, which is not part of the std::string API. If you can suggest
a better name, I'll gladly change it.

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

Are you sure? We're all friends and we're not supposed to be meddling
with the contents of stats, but this makes it possible to enforce the
policy.

> * Why is cow() public?

It doesn't hurt, and allows clients to anticipate to a String that
they'll be needing to modify data. IMO it is similar to allowing
reserve() to be public.

> * s/preAlloc/reserve/

Done.

> * s/memhandle/store_/

Done.

> And check other members for naming style. Use trailing underscores and
> mixedCase consistently, everywhere.

Ok.

> * s/alloc_strategy/calcCapacity/ or similar. The method does not allocate,
> change, or return any "strategy".

is "estimateCapacity" fine?

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

Done. Also added comments where deviating from the norm.

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

That's intentional. nreallocs tracks the history of a SBuf, not of its contents.

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

Ok.

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

There are four, and the major difference is the type of data being
appended (SBuf,char*+size, c-string, std::string). Plus one formatted

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

Removed together with isNull.

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

Will do.

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

On some platforms (windows at least) there are different allocators
for malloc() and operator new().
free(new object()) and delete(malloc(42)) are illegal on those platforms.
I think I've seen that Squid overrides operator new with xmalloc, but
I'm not 100% sure of that, and thus the double-copy.
Please let me know if it can safely be dropped.

I'll check into the remaining issues during the afternoon, fix what
has to be fixed and comment on the rest.

Thanks!

-- 
    /kinkie