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. > 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. 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. >> 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. >> 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. Cheers, Alex.
