Re: StringNG merge?

On 11/05/2012 08:24 AM, Alex Rousskov wrote:
> On 11/03/2012 06:56 AM, Amos Jeffries wrote:
>> On 4/11/2012 12:55 a.m., Kinkie wrote:
>>> Hi all,
>>> Is it maybe time to resuscitate StringNG, with the target of merging
>>> in time for 3.3?
>>
>> It is too late for 3.3 features and polish now.
>> But 3.4 is not scheduled for branching until next March and StringNG
>> should be able to make that IMO.
>
> I hope v3.4 will be branched sooner than March 2013, but I agree that
> StringNG is a good candidate for that version.
>
>
>>> If we agree, any preference on how to proceed?
>>
>> Waiting for that agreement. Then merge.
>
> I plan to review the current code this week. IIRC, there were doubts
> whether the new string buffers support header manipulation and I/O
> patterns we need. I need to double check that they do.

I found the thread discussing these problems. It looks like the
conclusion was that StringNG cannot support BodyPipe or similar I/O
patterns efficiently, but can still be useful elsewhere.

> +SBuf&
> +SBuf::append(const char * S, size_type pos, size_type n)
> +{
> + Must(pos==npos || pos >= 0);
> + Must(n==npos || n >= 0);
> +
> + //acts as "the number of bytes to copy"
> +

I do not understand what that comment is about. Is it out of date or
misplaced?

> +SBufList::SBufList()
> +{
> + //nothing to do really..
> +}
> +
> +SBufList::~SBufList()
> +{
> + //nothing to do really..
> +}
> +

Please remove these if they do nothing, especially since your destructor
adds a virtual table to this class.

> +/** split a SBuf into a list
> + *
> + * split a SBuf into a list, using any of the supplied delimiters as a
> + * separator.
> + * The separators are left at the BEGINNING of the split stings.
> + * For example: SBufSplit(SBuf("Foo Bar,Gazonk"),SBuf(" ,")) will return a list
> + * containing
> + * {SBuf("Foo"), SBuf(" Bar"), SBuf(",Gazonk")}
> + */
> +SBufList SBufSplit (SBuf tosplit, const SBuf & delimiters);

AFAICT, this does not split "tosplit". It creates a list while leaving
caller;s tosplit intact.

Why are we adding this strange function? I cannot find where it is used.

> +class StrList
> +{
> +public:
> + StrList() { }

This class is missing a description.

> + StrList(SBufList s) {
> + const SBuf sep(", ");
> + SBuf b=SBufListJoin(s,sep);
> + data_=b.toString();
> + }
> + StrList(const char *s) {
> + data_=s;
> + }
> + StrList (SBuf s) {

These single-parameter constructors are probably missing "explicit"
keyword to prevent accidental (and expensive!) conversions. Please also
check whether all of them need to copy their actual arguments instead of
using references.

Sorry, I ran out of time. More to come.

Alex.
Received on Sun Nov 11 2012 - 05:03:57 MST