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

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.

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

* 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"

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

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.

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.

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.

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)

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.

src/RequestFlags.h:
* on the constructor initialization list, please use one

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

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.

Amos
Received on Tue Sep 18 2012 - 08:01:48 MDT