Re: StringNG merge?

From: Kinkie <gkinkie_at_gmail.com>
Date: Sun, 11 Nov 2012 19:53:32 +0100

> 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.

It can probably be used as the foundation for that (e.g. via a
circular list of SBufs). But that will come later; worst case, we can
still use MemBuf for that.

>> +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?

Out of date. Removed.

>> +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.

Ok, removed.

>> +/** 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.

it's similar to PERL's split() function in purpose, and it can be used
to create simple tokenizers.

>> +class StrList
>> +{
>> +public:
>> + StrList() { }
>
> This class is missing a description.

It's an intermediate step, its purpose is to provide a path for
disambiguating String-used-as-a-list by the various strList*
functions.
It can be omitted from the merge.

>> + 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.

Added.
They do. StrList an intermediate step, to find and collecr all the
client code which does implicit string-to-strlist conversions.
It's a side project, which can be skipped for now.

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

Thank you.
Branch updated with the changes so far.

--
    /kinkie
Received on Sun Nov 11 2012 - 18:53:47 MST

This archive was generated by hypermail 2.2.0 : Mon Nov 12 2012 - 12:00:06 MST