Re: [MERGE] SBuf CacheMgr integration

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.
Received on Wed Oct 09 2013 - 15:49:03 MDT