On Wed, Sep 22, 2010 at 5:31 PM, Amos Jeffries <[email protected]> wrote: > 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:.
I'm fine either ways, the only client I'm concerned with is SBuf. >>>> _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". removed. >>>> _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 canAppend is exactly "isAtEnd && willFit" (isAtEnd is always true if RefCount==1) > or rather isAtEnd being a case of canAppend(0) Well, yes: willFit(0) is always true by definition (you can always append nothing). >>> 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. Most String/Buf implementations avoid locking, there's too much overhead to be spent for such a low-level function, it's usually better dealt with on an higher level. >>>> _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. Done. Thanks. -- /kinkie
