Re: StringNG merge?

On Sun, Nov 11, 2012 at 11:21 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 11/03/2012 05:55 AM, Kinkie wrote:
>
>> Feature-branch is at lp:~kinkie/squid/stringng
>
> I still find serious bugs in that branch, unfortunately (but not all of
> the comments below are serious bugs, of course):
>
>
>
>> === added file 'src/HttpHdrCc.h.moved'
>> --- src/HttpHdrCc.h.moved 1970-01-01 00:00:00 +0000
>> +++ src/HttpHdrCc.h.moved 2012-08-13 20:56:10 +0000
>> @@ -0,0 +1,56 @@
>> +/*
>> + * $Id$
>
> When I run "bzr send -o /tmp/bundle ../trunk" from the stringng branch
> and then look at the resulting bundle, it contains the above blob. If
> this is not some kind of bzr fluke, please remove src/HttpHdrCc.h.moved
> from your branch or double check that you are not modifying the code
> that is no longer there.

Fixed.

>
>> === modified file 'src/Makefile.am'
>> --- src/Makefile.am 2012-10-29 23:12:04 +0000
>> +++ src/Makefile.am 2012-11-01 21:35:44 +0000
> ...
>> @@ -142,7 +153,7 @@
>> UNLINKDSOURCE = unlinkd.h unlinkd.cc
>> UNLINKD = unlinkd
>> else
>> -UNLINKDSOURCE = unlinkd.h
>> +UNLINKDSOURCE =
>> UNLINKD =
>> endif
>>
>> @@ -259,6 +270,7 @@
>> CommCalls.h \
>> DescriptorSet.cc \
>> DescriptorSet.h \
>> + SquidConfig.cc \
>> SquidConfig.h \
>> SquidConfig.cc
>
>
> I see these and many other changes to src/Makefile.am, some of which are
> clearly buggy (like adding SquidConfig.cc twice above) and some quite
> significant and seemingly unrelated to StringNG scope. What is going on
> here? Is it possible that something when wrong during merging of
> Makefile.am changes? I know they are often difficult to get right.

Yes, they are. This is why I had proposed to apply the same sorting
principle to Makefile.am that we apply to headers: to make these
issues immediately apparent.
For my (and everyone else's) benefit, I've cobbled together a simple
checker: lp:~kinkie/support-tools/look-for-makefile-am-duplicates.pl

>> MemBlobStats::MemBlobStats(): alloc(0), live(0), append(0)
>> {}
>>
>> +MemBlobStats&
>> +MemBlobStats::operator += (const MemBlobStats& s)
>> +{
>> + alloc+=s.alloc;
>> + live+=s.live;
>> + append+=s.append;
>> + liveBytes+=s.liveBytes;
>> +
>> + return *this;
>> +}
>> +
>
> This is not a bug in this patch, but I noticed that MemBlobStats
> constructor does not initialize liveBytes (but the increment operator
> does increment it). Please fix in trunk.

Fixed.

>> +class OutOfBoundsException : public TextException
>> +{
>> +protected:
>> + SBuf _buf;
>> + SBuf::size_type _pos;
>> +
>
> Please move protected members after public ones (and private after
> protected).

Done.

>
>> + debugs(SBUF_DEBUGSECTION,DBG_DATA,
>> + id << " created, copyfrom="<<S.id);
> ...
>> + debugs(SBUF_DEBUGSECTION,DBG_DATA,
>> + id << " created, copyfrom string");
> ...
>> + debugs(SBUF_DEBUGSECTION,DBG_DATA,
>> + id << " created from c-string" <<
>> + ", pos=" << pos << ", n=" << n);
> ...
>> + debugs(SBUF_DEBUGSECTION,DBG_DATA,
>> + id << " destructed");
>
> These and probably others should not use DBG_DATA because the latter is
> reserved for a "large data dump" and they do not dump any string data at
> all.

Lowered level to 8.

>> + idx=s1.rfind(s5);
>> + rv=s1.scanf("%s , %d , %f",s,&i,&f);
>> + s=foxb;
>> + s=st.token();
>> + sng=sng.toLower();
>> + sng=foxb;
>> + const char *ref=t.rawContent();
>> + const char *match=t.rawContent();
>> + ref=match;
>> + for (SBufList::iterator i=res.begin(),j=control.begin(); i!=res.end(); i++,
> ...
>> + SBuf& append(const char * S, size_type pos = 0, size_type n = npos);
>> + SBuf& append(const std::string &string, size_type pos = 0, size_type n = npos);
>> + message = static_cast<char*>(xmalloc(buf.length()+1));
>> + for (i = begin(); i != end(); i++)
>> + virtual int_type overflow(int_type aChar = traits_type::eof()) {
>> *gross_size = pool ? StrPoolsAttrs[i].obj_size : net_size;
>> + foo = s1.rawContent();
>> + foo = s1.c_str();
>
> The above two groups of randomly selected lines that show different
> style of using "=" and other operators. Some of the patch code is using
> compact "a=b" style, the other is using readable "a = b" style (same for
> other operators). Sometimes a single line mixes two styles! Can you
> change it to use the readable style throughout? I understand when old
> Squid code contains different styles from different people, but should
> not we expect better consistency from a single-person patch?

Done ( a re-check may be needed in some auxiliary files)

> Same for separating call arguments. Sometimes you use compact "a,b" and
> sometimes "a, b".

Done (same caveat as above)

> I think most of these can be fixed with sed or similar, without a lot of
> manual work.

half-half. Not a big deal anyway.

>> + : store_(GetStorePrototype()),off_(0),len_(0)
>> +{
>> + Must(pos==npos || pos >= 0);
>> + Must(n==npos || n >= 0);
>> +
>> + if (S==NULL)
>> + n=0;
>> + if (n==npos)
>> + n=strlen(S)-pos;
>> +
>> + const char *actual_start=S+pos;
>> + debugs(SBUF_DEBUGSECTION,DBG_DATA,
>> + id << " created from c-string" <<
>> + ", pos=" << pos << ", n=" << n);
>> + store_=new MemBlob(estimateCapacity(n));
>> + if (n > 0)
>> + store_->append(actual_start,n);
>> + len_=n;
>> + ++stats.alloc;
>> + ++stats.allocFromCString;
>> + ++stats.live;
>
> and
>
>> +SBuf&
>> +SBuf::append(const char * S, size_type pos, size_type n)
>> +{
>> + Must(pos==npos || pos >= 0);
>> + Must(n==npos || n >= 0);
>> +
>> + if (S==NULL)
>> + return *this;
>> + if (n==npos)
>> + n=strlen(S)-pos;
>> +
>> + debugs(SBUF_DEBUGSECTION,7,
>> + id << MYNAME << ",S=c-string" <<
>> + ",pos=" << pos << ",n=" << n);
>> +
>> + reserve(n);
>> + const char *actual_start=S+pos;
>> + store_->append(actual_start,n);
>> + len_+=n;
>
> These are almost identical but there are subtle differences (including
> difference in problems!). Please make the constructor call
> SBuf::append() instead of repeating the same code. I do not think saving
> a couple of CPU cycles is worth introducing this difference in behavior
> (not to mention code duplication).

Done.

> The next two comments can be ignored if the above problem is addressed
> (because the problematic code will disappear).
>
>
>> + if (S==NULL)
>> + n=0;
>> + if (n==npos)
>> + n=strlen(S)-pos;
>> +
>> + const char *actual_start=S+pos;
>
> It is not kosher to add something to a NULL pointer and S may be NULL
> when we are calculating actual_start. I recommend removing actual_start
> because it is not REused. If you want to keep it, move it to where it is
> used.
>
>
>> + store_=new MemBlob(estimateCapacity(n));
>> + if (n > 0)
>> + store_->append(actual_start,n);
>
> Let's not allocate new store_ if we are not going to use it.

Doubly-useless, as the initializer list first assigns the
storeprototype - which is a valid empty blob.

>> + void reserve(size_type minsize);
>> +SBuf::reserve(size_type howmuch)
>
> Please use camelCase for minsize and howmuch. You may want to name them
> the same.

Done (minSize)

>> + SBuf& trim(const SBuf &toremove, bool atBeginning=true, bool atEnd=true);
>
> Please use camelCase for toRemove, especially if other parameters are
> using camelCase.

done.

> Please check other method parameter names for similar camelCase
> problems. "npos" should stay as is though, for STL compatibility reasons.

Ok.

>> +#else
>> + //enabling this code path, we try to release the store without deallocating
>> it.
>> + // will be lazily reallocated if needed.
>> + if (store_->LockCount() == 1)
>> + store_->size=0;
>> +#endif
>
> Please use store_->clear() instead of modifying the member we "must
> treat as read-only" according to the MemBlob API documentation.

Right. Done.

>> +SBuf&
>> +SBuf::append(const std::string &s, SBuf::size_type pos, SBuf::size_type n)
>> +{
>> + return append(s.data(),pos,(n==npos ? npos : s.length()));
>> +}
>
> Why are we overwriting caller-supplied value of "n" (not equal to npos)
> with s.length()? It looks like the third parameter calculation is wrong
> on many counts. Please fix carefully.

In fact, it's not needed at all. Removed that check.

>> + debugs(SBUF_DEBUGSECTION,DBG_DATA,HERE << "same memhandle and contents:
>> ");
>
> A column at the end implies that something will follow, but nothing does.
>
> Are you going to remove HERE from your code since trunk deprecates it now?

MYNAME and HERE should now be gone.

>> + debugs(SBUF_DEBUGSECTION,8,
>> + MYNAME << "this=" << (void *)this << "other=" << (void *)&S);
>
> Please do not use MYNAME in level-8 debugging -- it is reserved for
> levels zero and one. Please check the rest of the patch for this.

gone.

> "this" and "other=" are missing a space between them.
> reason to be inconsistent with the above code, please use "id".

done.

>> + if (n==npos)
>> + n=length();
>> + n=min(n,length());
>
> Please add "else" between the above two lines in SBuf::consume().

Done.

>> +SBuf::size_type
>> +SBuf::copy(char *aBuf, SBuf::size_type aBufLen) const
>> +{
>> + Must(aBufLen>0);
>> +
>
> This Must() is too restrictive. Copying an empty string into an empty
> buffer should be allowed (to avoid checks in callers for this special
> condition). You need aBufLen to be sufficient, not positive.
>
> Also, we usually use "a" prefix for constructor or setter parameters. In
> this case, aBuf and aBufLen are neither. Consider using "dest" and "n"
> instead.

Ok.

>> + /** Request to extend the SBuf's available store space.
>> + *
>> + * After the reserve request, the SBuf is guaranteed to have at least minsize
>> + * bytes of append-able backing store (on top of the currently-used portion)
>> + * \throw SBufTooBigException if the user tries to allocate too big a SBuf
>> + */
>> + void reserve(size_type minsize);
>
> I think it is a bad idea to make Sbuf::reserve() behave very differently
> from std::string::reserve(), especially when we are trying to make other
> methods behave the same. I suggest:
>
> 1) Add reserveCapacity() that behaves like std::string::reserve(). Do
> not change any callers.
>
> 2) Rename reserve() method to reserveSpace(). Do not change any callers.
>
> 3) Go and replace all reserve() calls with the right one.
>
> 4) Optionally, rename reserveCapacity() to reserve(). I would not do
> that unless you remove reserveSpace() completely as well.

I don't have the time to do this before the weekend. I'll leave this
open as a TODO

>
>
>> + * \param pos start sub-stringing from this byte. If it is
>> + * greater than the SBuf length, an empty SBuf is returned
>
> Please s/an empty SBuf is returned/Sbuf is emptied/ or similar to
> indicate that we do not simply return an empty string but clear the
> buffer itself.

Changed to "the SBuf is emptied and an empty SBuf is returned".

>> +SBuf::size_type
>> +SBuf::find(char c, SBuf::size_type startpos) const
>> +{
>> + if (startpos==npos)
>> + startpos=length();
>> + startpos=min(startpos,length());
>
> If startpos is npos, we can just return npos. If you do not want to do
> that for some reason, please add "else" between the above two last
> statements.
>
> Same for the other find() method.

returning npos in both cases.

Have to stop here, out of time. I'll continue next week.

Thanks!

    /kinkie
Received on Fri Nov 16 2012 - 09:23:04 MST