Re: [PATCH] String API re-implementation using SBuf

On 10/11/2013 11:03 PM, Amos Jeffries wrote:
> On 10/10/2013 10:12 p.m., Amos Jeffries wrote:
>> Replace String class internals with an SBuf.

FWIW, you made the review process more time consuming and less accurate
[for me] by moving implementation from .cc and .cci into .h. As you
know, bzr does not track such changes well. I only found a bug or two,
but I am not confident the move did not hide more problems from me.

I did not compare the moved strwordtok() implementation.

> + String(SBuf const &s) : buf_(s), defined_(s.length() > 0) {}

> + String &operator =(SBuf s) {
> + defined_=true;
> + buf_=s;
> + return *this;
> + }

> + SBuf toSBuf() const {return buf_;}

The above change SquidString API because SquidString did not have the
above methods before. Thus, I should probably review them as new code,
not re-implementation of SquidString:

* Manipulation of defined_ in the assignment operator is questionable
because it differs from similar manipulation in the constructor. If the
code is correct, a comment explaining the difference is warranted.

* The constructor is missing either an explicit keyword or a comment
explaining why the keyword is not used. If in doubt, use explicit, at
least for now.

* In the assignment operator and in toSBuf(), you may want to pass SBuf
by const reference.

> + char const * termedBuf() const {
> + if (!defined()) return NULL;
> + // XXX: callers will probably try and write to the buffer,
> + // but SquidString never offered any more guarantee than SBuf
> + // does now about buffers existence.
> + return buf_.c_str();
> + }

Can you give an example of a caller of this const method that writes to
the const buffer it returns? I hope we do not have a lot of such code in
Squid but your "probably will" wording seems to ruin that hope.

> + defined_|=(len > 0);

Please do not use bit operators on boolean members, even if they work.

> + int cmp(char const *s, size_type count) const {
> + // XXX: SquidString ignores the terminator. SBuf treats it as a byte.
> + if (s!=NULL && strlen(s) < count)
> + count = strlen(s);
> + return buf_.cmp(SBuf(s,count), count);
> + }

s may not be null-terminated in this context. We should not use
strlen(s), should we?

Why change the implementation here to use SBuf::cmp() and introduce bugs
and XXXs? Can we continue to use strncmp() instead? The goal is not to
produce a "better" SquidString but "functionally the same" SquidString,
right?

> + void cut(size_type newLength) {
> + if (newLength > buf_.length()) return;
> + if (buf_.length() == 0 && !defined()) return;
> + buf_.setAt(newLength, '\0');
> + buf_.chop(newLength);
> + }

As far as I can tell, chop() does not do what the author of the above
code thought it does. The cut() method is not supposed to change the
first SquidString character when newLength is positive, but the
re-implementation does. If I am right about this bug, this is a good
opportunity to rub in my earlier objections over chop() itself :-/.

HTH,

Alex.
Received on Tue Oct 15 2013 - 18:12:38 MDT