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

On 11/22/2013 08:25 AM, Amos Jeffries wrote:
>>> >> 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.

Done, but please note that removing external defined() callers will not
solve the termedBuf() problem. There are three cases from termedBuf()
caller point of view:

1. Caller expects defined() String to return a not-NULL c-string.
2. Caller expects an empty "" String to return a not-NULL c-string.
3. Caller expects !defined() String to return a NULL c-string.

Case #1 is handled the same before and after your changes.
Case #2 will be broken if you do not return buf_.c_str().
Case #3 will be broken if you return buf_.c_str().

We may not know for sure whether cases #2 and #3 exist, but we need to
pick between breaking case #2 or case #3. Due to termedBuf()
look-and-fill, and after quickly scanning termedBuf() callers, I suggest
that we break #3 and return buf_.c_str() after you double check those
callers and adjust those that need adjustments (if any).

Thank you,

Alex.
Received on Fri Nov 22 2013 - 16:49:44 MST