Re: StringNG merge?

On 11/03/2012 05:55 AM, Kinkie wrote:

> Feature-branch is at lp:~kinkie/squid/stringng

Found one or two more bugs and a few nits:

> + int c=(*this)[j];
> + if (isupper(c))
> + rv.setAt(j,tolower(c)); //will cow() if needed
...
> + int c=(*this)[j];

Please make "c" const if possible.

> +#include "OutOfBoundsException.h"
> +#include "SBufExceptions.h"
> +#include "base/RefCount.h"

These are out of order per current ordering rules. Please check other
includes.

I do not think reAlloc() should be inlined -- it is a heavy operation.

> +char *
> +SBuf::rawSpace(size_type minsize)
> +{
> + cow(minsize); //passing default npos is handled by cow
> + ++stats.rawAccess;
> + return bufEnd();
> +}

I do not think this matches rawSpace() specification. "minsize" is
referring to the size of space after content, not the total size of the
storage buffer. When you call cow(), you should specify the total buffer
size (length()+minsize?).

> + // we're not concerned about RefCounts here,
> + // the store knows the last-used portion. If
> + // it's available, we're effectively claiming ownership
> + // of it. If it's not, we need to go away (realloc)
> + if (store_->canAppend(off_+len_,howmuch)) {
> + debugs(SBUF_DEBUGSECTION, 7, HERE << "not growing");
> + return;
> + }

I think the above SBuf::reserve() may not offer what the developer
expects it to offer. The above comments are misleading because the code
does not claim any ownership (effectively or otherwise) -- the callers
have to do that. The code may lead to the very inefficiencies that
reserve() is meant to avoid! Here is an example:

    s2 = s1;

    ... many function calls later, a developer no longer knows that
        s1 and s2 share the same MemBlob B, but they do ...

    s1.reserve(64KB); // OK. B will be reAlloced if needed.
    s2.reserve(64KB); // No-op. B already has 32KB space.
    while (...) {
       s1.append(1 Byte); // OK, will use the reserved space.
       s2.append(1 Byte); // Will keep realloc-ing many times!
    }

Can reserve() call cow(length() + howmuch) instead? That way, since the
caller is reserving SBuf with the intent to change it, we are not
creating more work, but we do make the reservation optimization work.
Can you think of a counterexample where cow(length() + howmuch) will
result in worth performance than the current non-reserving reserve()
implementation?

> + reserve(1);
> + *(bufEnd())='\0';

I think the above code in c_str() can be simplified as

    *rawSpace(1) = '\0';

> +/** obtain prototype store
> + *
> + * Just-created SBufs all share to the same MemBlob.
> + * This call instantiates and returns it.
> + */
> +
> +MemBlob::Pointer
> +SBuf::GetStorePrototype()

I do not understand why this is called "prototype". It may return a
"protostore" or "protobuffer", but this is not related to types or
sketches. I suggest calling this Sbuf::DefaultStore() or
Sbuf::InitialStore(), but you may find a better name.

> + if (StorePrototype==0) {

Please use !StorePrototype (preferred) or compare with NULL (if you like
that a lot more).

Please declare StorePrototype inside SBuf::GetStorePrototype() if
possible. It should not be used outside that method, right? As a bonus,
you would not have to document it then.

> + _SQUID_INLINE_ static MemBlob::Pointer GetStorePrototype(); //only used by anonymous constructor

That comment is a lie although I am not sure what "anonymous
constructor" is. The method is used by multiple constructors.

> + message = static_cast<char*>(xmalloc(buf.length()+1));
> + buf.copy(message,buf.length());
> + message[buf.length()]='\0';

This code can be simplified by writing:

    message = xstrdup(buf.c_str());

> + NullSBufException(const char *aFilename = 0, int aLineNo = -1);
> + InvalidParamException(const char *aFilename = 0, int aLineNo = -1);
> + SBufTooBigException(const char *aFilename = 0, int aLineNo = -1);

If possible, please add "excplicit" to all of the above, to prevent
silent conversion from integer to an Exception.

src/ipc/TypedMsgHdr.cc appears to have changes unrelated to this project.

src/mem.cc has whitespace non-changes.

I assume you will drop StrList and friends from this submission so I did
not continue to review them.

I have not reviewed Tokenizer-related changes yet. I will wait for all
other issues to settle first. Feel free to exclude Tokenizer from the
proposed commit scope. It may be best to review Tokenizer when we also
have some Squid code written or adjusted to use it.

Thank you,

Alex.
Received on Mon Nov 12 2012 - 20:22:01 MST