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.
