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

On 16/10/2013 7:12 a.m., Alex Rousskov wrote:
> 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.

Okay, undone that moving now. The attached patch is more easily comparible.

> I did not compare the moved strwordtok() implementation.

It had not changed.

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

Done. Both use size()>0 now.

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

Done.

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

Done.

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

I can't sorry. There are too many cases of String being used and
termedBuf() used as a String -> char* converter for passing the char*
into deeply nested old code to be certain of what happens everywhere.
Thus the "probably". The only thing that seems certain is that String
has a comment in rawBuf() that the callers of that function had been
checked for not relying on 0-termination.

>
>> + defined_|=(len > 0);
> Please do not use bit operators on boolean members, even if they work.
>

Fixed.

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

Yes you are right. I was hoping to get all the functionality offloaded
to SBuf calls. But this one will have to revert.

>
>> + 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 :-/.

Right. I was being confused after looking at String::cut too long.

No fault of SBuf::chop particularly. I have redone it to follow the
logics order of the original more closely and use SBuf::substr() instead
of chop().

Amos
Received on Mon Nov 18 2013 - 12:13:17 MST