Re: StringNG merge?

On 11/11/2012 11:53 AM, Kinkie wrote:
>>> +/** 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.

Perl's split() is a nice function, but SBufSplit() delimiters-preserving
behavior is ugly, especially if its purpose is to create simple
tokenizers. Since SBufSplit is not used (unless I missed such uses), I
recommend removing it. When we actually need something like this, you
may add it back, of course, and we can then discuss whether it is the
best tool for the job at that time.

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

It is your call whether to include it or not. If it is not needed for
the rest of the patch, consider removing it -- it is much better to
evaluate a class when there are examples of how it is going to be used.

If you decide to keep it, it must be properly documented and implemented
(regardless of whether you plan to remove it eventually).

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

That does not explain why "SbufList" and "SBuf" constructor arguments
are passed by value. I am not saying they should not be. There may be a
reason for those copies.

If these classes are needed to detect implicit string-to-strlist
conversions _and_ you plan to remove/replace such conversions (after
they are detected), then perhaps these classes are not needed at all or
do not need to be implemented (so that the linker will catch forgotten
conversion cases)?

If this is a separate project/step, consider removing it from StringNG
submission so that there is fewer things to argue about.

Another concern about SBufList is that it provides nothing but two
search functions, and those search functions do not rely on the storage
being a specific std::list<>. There seems to be no reason to restrict
that search functionality. Most likely, they should be implemented as
global functions, accepting any container that we can iterate, AND/OR as
a comparison function for a container type (depending on how they are
actually used -- something, again, we cannot see right now).

Thank you,

Alex.
Received on Sun Nov 11 2012 - 19:54:39 MST