On 19/09/2012 2:36 a.m., Kinkie wrote:
On Tue, Sep 18, 2012 at 10:01 AM, Amos Jeffries <[email protected]> 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
