Re: [MERGE] SBuf CacheMgr integration

On 10/10/2013 5:14 a.m., Kinkie wrote:
> 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!
>

+1. This version looks fine to me.

Amos
Received on Thu Oct 10 2013 - 02:11:59 MDT