Re: [RFC] Time to talk about StringNG merge again?

>>> That comment is probably stale because canAppend() is currently not
>>> related to ownership of the buffer: It will happily return true for
>>> shared buffers. AFAICT, reserveSpace() should be implemented using
>>> something like this:
>>>
>>> Must(0 <= minSpace && minSpace <= maxSize - length());
>>> reserveCapacity(length() + minSpace);
>>
>> The comment is badly worded but the code is correct: canAppend() tells
>> us if the end of our buffer is at the end of the used portion of the
>> MemBlob, and there is enough space in the unused MemBlob portion to
>> contain the data we plan to use. If the buffer is shared, this
>> guarantee may become stale at the next append operation to any SBuf
>> sharing the same MemBlob.
>>
>> A decision needs to be made if these two methods have different
>> purposes or are to be simply different expressions of the same
>> underlying concept (as it is now).
>> IMO, thinking about it, it does make sense that reserveSpace also
>> guarantees that after the call the MemBlob will be exclusively owned
>> by this SBuf (aka, blindly cow()), while reserveCapacity can mostly be
>> meant as an optimization, to hint the SBuf memory management system
>> that an append operation of a known size is about to happen, so that
>> only one cow is performed.
>> What do you think?
>
>
> Sounds good.
>
>
>>
>>> The above two changes imply that reserve*() methods ensure
>>> single-ownership of the buffer by the caller. This is not something
>>> documentation of those methods currently guarantees, but I think it is
>>> logical to assume that anybody calling one of these two methods wants to
>>> modify the string and, hence, will need exclusive control over that
>>> string. I cannot think of any reason to call those methods otherwise.
>>
>> See above.
>>
>>>> typedef int32_t size_type;
>>>
>>> Would it be better to make this unsigned? I do not think string sizes
>>> can go negative. std::string is using an unsigned type, right? FWIW,
>>> whenever I try to use MemBlob that has a signed size_type as well, I am
>>> forced to do casts that would not exist otherwise. I think both types
>>> should be changed to reflect what they are representing (a size of a
>>> buffer).
>>
>> I have the same doubt. I'm just a bit afraid to change this late. Will
>> try to do as an isolated change. So far this is NOT done.
>>
>>> On another note, would it be better to use MemBlob::size_type instead of
>>> int32_t?
>>
>> currently they're both int32_t. I believe it's more readable with the
>> extra level of indirection, at the cost of having to manually keep
>> them in sync.
>> Added a comment to highlight this fact.
>
>
> So long as SBuf is backed by MemBlob defining SBuf::size_type to be
> MemBlob::size_type makes more sense. One who needs to know can always look
> up both (probably should). The compiler will tell about signedness errors in
> new/modified code very quickly.

Ok, changed to reference MemBlob::size_type instead.

> The problem will be converting from signed to unsigned at this stage. I
> think get the rest to a milestone we can agree on merging before attempting
> please. Then if everthing goes sour in the change we can still revert and
> merge with a TODO about perfecting the polish.
> When you do the change ensure you have a latest GCC or clang you can use,
> they will be far more pedantic about signedness comparisons and iterator
> ranges etc where the issues are most likey to occur.

Ok.
Most of my development is done on Ubuntu Raring, that should be fresh enough.

>>> Also, I suggest splitting this into two methods, one with a required
>>> (first?) SBufCaseSensitive parameter and one without it. This will allow
>>> callers to specify n without specifying isCaseSensitive and vice versa.
>>> The shorter, inlined method will simply call the longer one with
>>> caseSensitive, of course, so the implementation will not require more
>>> code or performance overhead.
>>
>> Ok. I'm calling the two shorthand versions cmp and casecmp respectively
>> (please let me know if you'd prefer the naming-convention compliant
>> caseCmp instead)
>
> Please. We need semantic similarity to std::string not symbolic.

I take this as a "please yes".

>
>
>> Code is as before at lp:~squid/squid/stringng-cherrypick, ready and in
>> sync with trunk.
>> test-suite runs OK on eu and ubuntu raring.
>>
>> I'll be on holiday in a couple of days for a couple of weeks.

In the meantime, I've found quite a few issues with \0-cleanliness and
parameter auto-promotion; I've also noticed that I've messed up the
commits crossing things with an older branch (where things were
silently failing as it didn't contain Alex' extensive unit tests).,

I'll have to tidy things up, current status is "not done" :(

--
    /kinkie