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

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!
>>
>
> I see you meant typedefs.h / structs.h not protos.

Yes, sorry.

> Some quick audit results:
>
> * This patch adds double-empty lines again. Please run your double-empty
> removal across at least:
> src/CacheMgrPasswd.cc src/CacheMgrPasswd.h src/HttpHeaderFieldInfo.h
> src/HttpHeaderFieldInfo.h src/HttpHeaderFieldStat.h src/RequestFlags.cc
> src/SquidConfig.cc src/SquidConfig.h src/YesNoNone.h
> src/acl/AclDenyInfoList.h src/log/CustomLog.h src/ssl/context_storage.h

fixed.

> * our class definitions usually have the first { on a separate line from
> "class Foo". Please do that for consistency. There are a lot of classes
> being added by this patch with the format "class Foo {\n"

Whoops, fixed.

> 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.

> src/HttpHeaderFieldInfo.h:
> * please do not add spaces between function name and ( for parameters in any
> new code. Thi swill also help with the constructor line wrapping.

Ok, thanks

> src/HttpHeaderFieldStat.h (same in src/store_rebuild.h):
> * why are you using '#' inside a C-comment ? if you mean "number of" please
> use the words. Clarity is important even for one-liners.

I don't recall changing that, but sure.

> 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?

> 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?

> src/RequestFlags.cc:
> * please use HERE in all the debugs which are just dumping the function
> name. They are currently displaying the wrong/old class details.

Done.

> 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.

> src/acl/AclAddress.h:
> * this is still struct where elsewhere you have converted to public class.

Right. Fixed.

> 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.

-- 
    /kinkie