Re: [RFC] Interim review: typedefs.h / protos.h splitting

On 19/09/2012 2:36 a.m., Kinkie wrote:
> On Tue, Sep 18, 2012 at 10:01 AM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
>> On 18/09/2012 1:47 a.m., Kinkie wrote:
>>> Hi all,
>>> Attached you'll find an interim patch aiming at replicating for
>>> typedefs.h and protos.h the same work I've done for protos.h.
>>>
>>> It
>>> - separates structures declarattions into own header files
>>> - transform typedef structs (and plain structs) into classes
>>> - renames some types to CamelCase convention
>>> - adds some (for now empty placeholders) code files for c++-ification
>>> of classes (e.g. CacheMgrPasswd, CustomLog)
>>> - implements the full set of getters/setters for class RequestFlags -
>>> the method names are certainly improveable, advice is sought
>>> - changes some linkage from C to C++
>>> - implements some default constructors and fixes some which were
>>> missing some data members
>>>
>>> I'd like to get some feedback before committing more effort to this;
>>> also an interim merge could be useful - the patch is already more than
>>> 10klines long.
>>>
>>> Thanks!
>>>
>> CacheMgrPassword.h:
>> * Please use Mgr:: namespace for any new or majorly altered cache manager
>> objects. CacheMgrPassword should be Mgr::ActionPasswordList I think (we will
>> have to redesign later for Mgr::Privileges to be more useful).
> That's more than I was hoping to act on on this first step, but ok.

You are renaming and shuffling already. That is all I ask for, get it in
a useful position. Re-design can happen later in a searate SourceLayout
upgrade.

>
>> src/HttpHeaderTools.h:
>> * if you are sorting the class pre-defines in chunk @94,10 - please sort
>> alphabetically, not just shunting the line to another un-sorted position.
> Sure. I suspect they may like to get some automated tools like includes?

Maybe. Not as part of this though.

>> src/PeerDigest.h:
>> * if DigestFetchState was updated to use the CbcPointer to CachePeer like it
>> should we would have circular reference locks happening. Why is this
>> reference needed at all? (dont change in this update, just something to look
>> into and see if w can drop as a separate fix)
> Do you want me to mark an XXX?

if it turns out to be a bug. a note abouut why it exists is needed at
minimum.

>
>> src/RequestFlags.h:
>> * on the constructor initialization list, please use one
> I suspect that this sentence is incomplete. I've fixed the
> indentation, but I don't know what else you meant.

Arg. Yes. I was meaning to say please use one line per initializer. So
we can trivially add/remove them and not have to work around line
wrapping so much.

>> When you have a class definition which needs no methods or definitions you
>> don't need to create a .cc file for it. Listing the .h in makefile
>> dependency lists is enough to distribute it correctly. Looking at
>> CustomLog.cc and a few others for this.
> This is intentional, really: as a follow-up to this activity I'd like
> to work towards more c++-ification: we still have lots of C-style
> functions (also constructors and destructors), and lots of
> linked-lists which would probably be better off as std::list. For
> CustomLog it'd be constructors and destructor(s), for instance.
> I can of course remove them and add them back later if you prefer.
>
> Attaching version 2 of the patch.

Fair enough if you are intending to do that c++-ification as next stage
of thise before mergign to trunk. However adding them to trunk before
havingcompleted that would be a waste.

Amos
Received on Wed Sep 19 2012 - 00:06:43 MDT