On 22/09/10 21:30, Kinkie wrote:
On Tue, Sep 21, 2010 at 4:18 AM, Alex Rousskov
<[email protected]> wrote:
On 09/03/2010 09:21 AM, Kinkie wrote:
You'll find the branch at lp:~kinkie/squid/stringng
I decided to take a look at MemBlob because it is used by SBuf which was
reviewed a few days ago. It would be nice to get a fixed SBuf committed, but
that would require committing the classes it uses.
Committing smaller self-contained parts may be an overall better strategy
than trying to fix everything first and then do one huge commit.
So here is a MemBlob review:
<snip>
Remove the "buf" prefix until you have some other size and capacity members
to distinction from. Besides, this is not a "buf". This is a "blob" :-)
Yup. Relics... I've been working on this for more than two years now.
I would not leave these data members public, but it is your call.
This class originally was private to SBuf. I've now clarified in the
class definition that it's not meant for general use; its only client
should be SBuf.
If still relevant that would be reason for protected: and friend
declarations in the class definition instead of public:.
_SQUID_INLINE_ void deleteSelf();
I hope this can be removed. If not, it must be documented. The corresponding
comments in .cci do not make sense to me and do not match RefCount.h in
trunk, as far as I can tell.
http://old.squid-cache.org/Doc/Prog-Guide/prog-guide-24.html
http://markmail.org/message/kt7357pwttmzrey3
Maybe the documentaiton is outdated?
Seems to be. The RefCountable classes no longer have "deleteSelf".
_SQUID_INLINE_ bool isAtEnd(const char *ptr) const;
Missing documentation.
Why is this needed? Some transitional call? If yes, document as such.
It is needed to know whether a the tail of a SBuf is at the tail of a
shared MemBlob. Such a SBuf can append without needing a cow, if
there's enough headroom in the MemBlob.
So, a char* way of saying:
if (SBuf::off_+Sbuf::len_ == MemBlob::size &&
SBuf::off_+Sbuf::len_ < MemBlob::capacity) {
...
with canAppend being a duplicate as above but <= MemBlob::capacity+N
or rather isAtEnd being a case of canAppend(0)
This reads like a command to free some space.
Consider: spaceSize()
spaceSize() would be more consistent with other Squid code as well.
_SQUID_INLINE_ bool canGrow(const size_type howmuch) const;
Missing documentation.
Usually unsafe and wastefull. See similar comments for SBuf.
It'd be unsafe in a concurrent (multithreaded) environment; how would
it be unsafe in a single-threaded env?
Squid won't be single-threaded for much longer. At least on the
timescale for string-level changes.
_SQUID_INLINE_ bool canAppend(char const * bufEnd, const size_type
howmuch) const;
Missing documentation.
Especially since it is not clear why canAppend() needs a pointer.
Looking at the .cci implementation, I question why would the caller
keep the bufEnd pointer if MemBlob manages it already? I do not think this
call or duplicated "bufEnd" pointer is needed.
if many SBufs reference the same MemBlob, an append operation to one
of them does not need a cow if the appended-to SBuf is at the tail of
the MemBlob and there is enough free space at the end of the MemBlob
to hold the data being appended.
This canAppend call checks exactly that. the SBuf knows its own tail,
while the MemBlob knows the tail of the used portion of itself.
see isAtEnd() comment above.
/**
* constructor, creates an empty MemBlob of size>= requested
* \param size the minimum size to be guarranteed from the MemBlob
*/
MemBlob::MemBlob(const MemBlob::size_type size)
{
debugs(MEMBLOB_DEBUGSECTION,9,HERE<< "size="<< size);
memAlloc(size);
}
I would recommend initializing mem and sizes to NULL/0s explicitly in all
the constructors. It does not cost much and may save a few hours of
debugging if things go wrong.
Isn't setting them in memAlloc enough? Besides the actual size may be
different (or would you mean to set them twice?)
In the constructor init list.
You can then enforce the MUST and ONLY requirements with
assert(mem==NULL) etc at the start of memAlloc.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.8
Beta testers wanted for 3.2.0.2
