Re: StringNG merge?

From: Kinkie <gkinkie_at_gmail.com>
Date: Sun, 4 Nov 2012 10:04:47 +0100

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

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

Thanks!

--
    /kinkie
Received on Sun Nov 04 2012 - 09:04:54 MST

This archive was generated by hypermail 2.2.0 : Mon Nov 05 2012 - 12:00:04 MST