On 10/10/2013 5:14 a.m., Kinkie wrote:
On Wed, Oct 9, 2013 at 5:48 PM, Alex Rousskov
<rouss...@measurement-factory.com> wrote:
On 10/09/2013 09:15 AM, Kinkie wrote:
Hi,
the files after the changes below can be most easily browsed at
http://bazaar.launchpad.net/~squid/squid/stringng/view/head:/src/SBufStatsAction.h
http://bazaar.launchpad.net/~squid/squid/stringng/view/head:/src/SBufStatsAction.cc
The code has been build- and run- tested.
On Sat, Oct 5, 2013 at 9:41 PM, Alex Rousskov wrote:
The "This file contains ..." comments do not seem to correspond to the
contents of the files.
Removed and replaced with a simple doxygen comment for the class itself.
You missed the "s" at the end of the "comments" :-). The .cc file has
the same bogus comment that you did not remove.
Damn. You're right.
Done; I couldn't find any template for Create and constructor, so I
left them empty as they are in all other Mgr::Action subclasses I've
checked.
Create is ClassActionCreationHandler (or OBJH), but its name is not
restricted by the API. There are no restrictions for the constructor
because it is not called by external classes (it can probably even be
private).
IMO, it is best not to document constructors unless they are doing
something tricky/unusual/confusing. They are documented by C++ itself: a
constructor is a function creating an object of the given class.
Ok.
Ideally, Create() should be documented as
/// Mgr::ClassActionCreationHandler for Mgr::RegisterAction()
It is sad that you have to use "others are doing this too!" as the only
excuse for violating documentation policy, but it is not a big deal, of
course.
Heh. Actually that's not the spirit had in mind, it was more an "I
haven't had the time to re-review in more detail the API, and I
couldn't resort to the very honorable art of copying because I
couldn't locate anything to copy from :P"
Debug messages in class methods need to be polished to remove empty
stings (if they are needed at all).
The purpose is to display the HERE string. Is there any better way?
Yes, removing the empty "" string will still display HERE. The third
debugs() parameter is optional.
I'm sorry, but it doesn't seem so either from reading the code and
from an actual attempt:
debugs(28,8);
results in:
SBufStatsAction.cc:49:16: error: macro "debugs" requires 3 arguments,
but only 2 given
This is irrelevant for the case at hand, as those debugs messages are
really pointless. Maybe making this optional parameter is an extension
in some branch other than trunk?
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.
It's an interesting pattern; I'd add it as the sanctioned way to
register actions.
Well, if you work on this, then it is best to make Mgr::RegisterAction()
return an action or at least a boolean value to simplify the pattern.
Thank you for addressing my comments. The remaining polishing can be
done during commit without review as far as I am concerned, but Amos
wanted to review this as well IIRC.
Not a problem, I can wait for his feedback.
In the meantime I've addressed the remaining points.
Thanks!
+1. This version looks fine to me.
Amos
