On 10/26/2012 05:21 AM, Amos Jeffries wrote:
> This polishes the RefCount API a bit.
>
> * Shuffle RefCount.h and its unit-tests into src/base/
Great!
> * Reworks struct Refcountable_ into class LockableObject in its own
> header file
> + changing the reference counter accessors to a lock()/unlock() names
> + some minor symbol updates of code directly utilizing the
> RefCountable_ members
Please s/LockableObject/Lock/g.
> +/**
> + * Base class for all reference tracking types.
> + * RefCount, CbData, etc
> + *
> + * The objects to be tracked inherit from this and
> + * construct/destruct/invalidation of the trackign mechanism
> + * use this interface API to maintain the locks.
> + */
> +class LockableObject {
It would be nice to describe what this class is before we jump into
describing what other classes are (or will be) using it [for]. For
example, you can start by saying something like "Something that can be
locked and unlocked. Supports multiple lock levels (handy for shared
objects). Must not be destroyed in a locked state but does not require
destruction when the lock level reaches zero."
Is the "construct/destruct/invalidation of the trackign mechanism..."
part missing some words? I cannot parse it.
"trackign" is misspelled.
s/interface API/API/ or s/interface API/interface/
> +private:
> + mutable unsigned count_;
> +};
Missing documentation. Consider "///< lock level" or similar.
> With this we can begin the process of replacing our multiple different
> implementations of the reference-counting pattern using LockableObject.
Sounds good, but I am not sure there is clear understanding and/or
consensus of what those implementations should be, so if you are going
to work on that, please post RFCs or sketches before spending a lot of
cycles on large-scope changes.
> No code changes have been made. Just symbol polishing.
> If there are no objections I will apply this in a day or two.
No objections from me.
> TODO: Followup stages which can build on this are (in no particular order):
> * update the backend of CBDATA to utilize LockableObject for reference
> tracking
Yes, but this is one of those tricky/controversial areas I am worried
about. I doubt cbdata API should be changed just for the sake of reusing
Lock.
> * update the HttpMsg locking API to utilize LockableObject for
> reference tracking
> * replace the HttpMsgPointerT locking API with RefCount<HttpMsg>
These should be pretty straightforward.
> * replace the StoreEntry locking API and move to CbcPointer
An even bigger mine field! Needs a lot of thought. I doubt, for example,
that StoreEntries should be cbdata-protected.
> I'm sure there are others floating around as well that I have not
> encountered yet. lock/unlock seems to be a popular operation in the code.
but often with different semantics so we must tread carefully.
Thank you,
Alex.
