Re: [MERGE] SBuf CacheMgr integration

On Wed, Oct 9, 2013 at 5:48 PM, Alex Rousskov
<rousskov_at_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!

-- 
    /kinkie