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

On 22/11/2013 8:20 a.m., Alex Rousskov wrote:
> On 11/18/2013 05:13 AM, Amos Jeffries wrote:
>> 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.
>
> Thank you. I am using the patch from your recent posting on the same thread.
>
>
>> - String(char const *);
>> + explicit String(char const *);
>
> Most of the non-String.* changes in the patch are due to this change.
> Most of those changes are not in a performance-sensitive code. And many
> are due to some _other_ code using char* types. Furthermore:
>
>> String &operator =(char const *);
>
> The above operator is inconsistent with an explicit char* constructor.
> If you do not allow implicit construction, you should not allow
> assignment either because it is just one more form of an implicit char*
> conversion.
>
> Please consider postponing adding "explicit" to the old constructor
> (which is unrelated to the patch scope AFAICT) until more stuff is
> converted to SBuf.
>
>
> The above comment does not apply to the explicit SBuf constructor. I am
> not sure "explicit" is warranted there, but I do not have enough
> ammunition to object to it: My overall motivation here is to make the
> eventual transition to SBuf smoother. Explicit constructors for an
> essentially temporary String class hurt that goal: We are forced to add
> String() wrappers first and then will be forced to remove them when the
> String class is gone...
>
>
>>> * 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.
>
> There is still a difference with:
>
>> +String::String(char const *aString) :
>> + buf_(aString),
>> + defined_(aString != NULL)
>
> Above, defined_ for "" is true. In the new SBuf constructor, defined_
> for "" is false. If the code is correct, a comment explaining the
> difference is warranted.
>
>
>> + if (str) {
>> + append(str);
>> + // XXX: empty string "" sent to append means no change,
>
> The comment is misleading a bit: Appending an empty string sets defined_
> to true in the patched code because patched String::append() sets
> defined_ to true unconditionally.
>
>
>> char const *
>> String::termedBuf() const
>> {
>> - return buf_;
>> + if (!defined()) return NULL;
>
> Since an empty String could have been defined() in the unpatched code, I
> suggest relaxing this to always return c_str(), even for undefined
> Strings. I think it will be safer this way, but you should double check
> that I did not miss any cases where it would break something.
>

I've gone through the non-String code use of String::defined() and tried
to find other ways of avoiding the defined()/NULL testing for ambiguous
cases of String/SBuf differences.

If you can check this attached patch is okay for merge to trunk we can
drop the nasty defined_ tracking the conversion patch has to do.

Amos