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
