Re: StringNG merge?

On 4/11/2012 10:04 p.m., Kinkie wrote:
>> Nits: (please fix before merge, but no extra review needed after changing)
>> * can you remove the extra whitespace around function name and paramater
>> list '(' characters please.
>> ** I am seeing a bunch of "foo( type" and foo (type" variantions on
>> methods.
> Will do
>
>> The terminate() problem I want to get rid of is still there.... my
>> objection is that we do not actually need it to exist as a visible function
>> at all.
> It's private, with a name of its own for code-deduplication (it's
> called in c_str() and scanf()).
>
>> 1) line 707 please use c_str() instead of buf() to access the nul-terminated
>> buffer.
>> c_str() produces a terminated buffer, so line 706 call to terminate() is
>> now redundant.
> Ok, that's a solution too :)
>
>> * If you want to do "--stats.rawAccess" in SBuf::scanf() to offset the
>> rawAccess counting c_str() does, fine. But I'm inclined to think of *scanf()
>> as a bad thing to call anyway. The inducement to remove it could be helpful.
> Hopefully we'll be able to remove it in favor of other things (e.g.
> SBufStream), but there's much legacy in squid.

Good.

>
>> 3) remove the unused symbol SBuf::terminate().
>>
>>
>> This hides the terminate() availability from public code documentation,
>> pushing people towards c_str() instead which is what we want to do.
>> It also inflates the rawAccess counter stats to show that vsscanf()
>> operations are working on the raw buffer in a bad way and should be avoided
>> when possible.
> I don't understand if the problem is with terminate() existing or with
> the documentation seeming to hint that it is public (the latter being
> of course easier to fix than the former).

My problem was with its existence. No need for it to hang around since
c_str() provides all the needs of termination-dependent code.

Amos
Received on Mon Nov 05 2012 - 09:06:47 MST