Re: StringNG merge?

On 11/15/2012 10:06 AM, Kinkie wrote:
>>>>> +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).
>
> Documented as "work in progress, incomplete, don't use" and #if 0-ed.
> Will be removed if you think this is not enough.

I guess I do not understand the motivation here. Why should we commit a
relatively large, complex, currently unneeded, unreviewed, disabled,
work-in-progress code to trunk? If there are some very good reasons why
you want this code committed, let's discuss them. Otherwise, it should
not be committed.

FWIW, I do not think it will make merging your stuff significantly more
complex. Since the code is relatively isolated and unused, manually
deleting most of it from the patch should be easy.

>> 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).
>
> yes, they could be made global. I am a bit less unsure about the
> refactoring strategy.
> The strategy I was aiming for IIRC was:
> - define a specific type to sweep/mark all uses of String as a StrList
> (while still keeping the underlying implementation a String)
> - change the underlying implementation from a String to a SBufList
> - profit
>
> Suggestions are welcome on a more effective strategy (but it can wait
> for another day; now I am aiming for SBuf, this is a side-project
> which will become relevant when we start porting over String to SBuf).

Yes, let's discuss this after the primary code is committed.

Thank you,

Alex.
Received on Thu Nov 15 2012 - 19:08:53 MST