On 10/04/2013 09:43 AM, Kinkie wrote:
> My recommendation would be to merge SBufExtras.* next, as it contains
> the CacheMgr integration. This will allow us to measure SBuf's
> effectiveness and possibly tune it as we start relying on it.
Hi Kinkie,
Please note that SBuf stats collection code was not really reviewed
because (IIRC) you wanted to experiment with it before discussing its
pros and cons. We may want to warn admins that this is an experimental
feature and they may avoid trusting those stats for now.
> In order to have the cleanest possible merge path, I ask whoever wants
> to review things to check out lp:~squid/squid/stringng. The relevant files
> are
> src/SBufExtras.* (they should probably be renamed, suggestions for a
> new name are welcome).
I found one class in src/SBufExtras.h. The files should be renamed to
match that class name.
The single-parameter class constructor is missing "explicit".
The "data" data member is not needed. Explode it to store stats objects
directly. It is better to add a pair of get/putPod() calls than to
introduce this wrapper structure that is otherwise unused.
The "This file contains ..." comments do not seem to correspond to the
contents of the files.
The class itself should be documented. The class members should be
documented. You may be able to make all its members except Create()
non-public and document all members except Create() and constructor as
/* Mgr::Action API */.
There are a few easy-to-fix code formatting problems as well like
missing empty lines between method definitions.
Debug messages in class methods need to be polished to remove empty
stings (if they are needed at all).
SBufStatsRegistrationHelperObject class is not needed. You can call the
registration function while initializing a boolean static variable. For
example:
static const bool Registered = (Mgr::RegisterAction(...), true);
If this is a common pattern, we should make Mgr::RegisterAction() return
an action or at least a boolean value to avoid the (..., true) noise.
The Must() in SBufStatsAction::dump() is probably not needed because
that method does not dereference the entry pointer.
In the future, please try to review code before submitting it for
merging and use [PATCH] or a similar subject to mark the email as code
submission.
http://wiki.squid-cache.org/MergeProcedure
http://wiki.squid-cache.org/Squid3CodingGuidelines
Thank you,
Alex.
