On Mon, Dec 2, 2013 at 1:36 AM, Alex Rousskov
<rouss...@measurement-factory.com> wrote:
> On 12/01/2013 02:41 PM, Kinkie wrote:
>> On Fri, Nov 29, 2013 at 2:26 AM, Alex Rousskov wrote:
>>> On 11/26/2013 02:28 AM, Kinkie wrote:
>>>
>>>> +bool isMember(const SBufList &, const SBuf &, SBufCaseSensitive
>>>> isCaseSensitive = caseSensitive);
>
> The last parameter can be const as well.
Done.
>>>> +bool
>>>> +isMember(const SBufList & sl, const SBuf &S, SBufCaseSensitive
>>>> case_sensitive)
>>>> +{
>>>> + for (SBufList::const_iterator i = sl.begin(); i != sl.end(); ++i)
>>>> + if (i->compare(S,case_sensitive) == 0)
>>>> + return true;
>>>> + return false;
>>>> +}
>>>
>>> STL already provides std::find() and std::find_if() that are more
>>> flexible/universal and sometimes faster than isMember(). I suggest that
>>> you provide a predicate to be used for case-specific comparisons of
>>> buffers instead of isMember(). Such a predicate would be usable with any
>>> SBuf container, not just SBufList.
>>
>> I suspect that these better belong to SBuf.cc than to SBufList.cc then.
>> Done so, and with perfect feature creep made it parametric (case
>> sensitive/insensitive) and built one also for prefix match.
>
> This is not feature creep IMO. This is simply the Right Way to support
> SBuf search in STL containers.
Ok. I'm taking this chance to learn :)
>> +class SBufComparePredicate {
>
> This only compares for equality. I suggest naming it SBufEqual.
Ok
>> +class SBufPrefixComparePredicate {
>
> This only compare for prefix-equality. I suggest naming it SBufStartsWith.
>
> If you think an explicit "Predicate" suffix is needed for some reason,
> add that or just "P". Personally, I do not see a need for either.
Ok.
> Please document each predicate class. Currently only the first is
> documented. A one-line description is enough for each IMO. For example:
>
> /// SBuf equality predicate for STL algorithms and such
>
> /// SBuf "starts with" predicate for STL algorithms and such
Excellent. Done.
>> + SBufPrefixComparePredicate(); //no default constructor
>> + SBufComparePredicate(); //no default constructor
>
> Why declare one if it should not be available? The compiler will not
> generate one because you already have a non-default constructor.
Ok
>> +class SBufPrefixComparePredicate {
>> +public:
>> + explicit SBufPrefixComparePredicate(const SBuf s, SBufCaseSensitive
>> caseSensitive = caseSensitive) :
>> + compareWith(s), sensitive(caseSensitive) {}
>> + inline bool operator() (const SBuf & checking) { return 0 ==
>> checking.startsWith(compareWith,sensitive); }
>> +private:
>> + SBuf compareWith;
>
> I would rename compareWith member in this class to "prefix". This will
> help document what the constructor parameter is meant to be.
renamed SBufEqual's parameter to "reference" and this to "prefix".
Used underscors suffix for private data members.
>> + explicit SBufComparePredicate(const SBuf s, SBufCaseSensitive
>> caseSensitive = caseSensitive) :
>> + explicit SBufPrefixComparePredicate(const SBuf s, SBufCaseSensitive
>> caseSensitive = caseSensitive) :
>
> * "s" can be a reference to avoid an extra refcounting overhead.
Yes.
> * caseSensitive can be const
Ok.
> * please use a unique name for the formal parameter; I suggest "sensitivity"
>
>
>> + SBufCaseSensitive sensitive;
>
>> + SBufCaseSensitive sensitive;
>
> I suggest sensitivity_ for both names.
What about caseSensitivity for the parameter, and sensitivity_ for the
private data member?
I notice only now the overlap between the parameter name and the value.
> BTW, the SBufCaseSensitive type should be similarly renamed as well IMO.
> Why is it even declared outside of the SBuf class?
Any suggestion is welcome, but I'd do that as a separate commit; for
now please let's leave it as is, I'm adding a TODO.
>> + inline bool operator() (const SBuf & checking) { return 0 ==
>> checking.compare(compareWith,sensitive); }
>
>> + return sl.end() != std::find_if(sl.begin(), sl.end(),
>> + SBufComparePredicate(S,case_sensitive));
>
>
> .arguments == of order natural a use Please
Done so I have.
>> + inline bool operator() (const SBuf & checking) { return 0 ==
>> checking.startsWith(compareWith,sensitive); }
>
> startsWith() returns bool. No need to compare it with anything.
Yes. That was broken. Sorry.
>> Do you agree these two small auxiliary classes belong to SBuf.h or do
>> you think they should go elsewhere?
>
> SBuf.h is fine IMO.
Ok.
>> + //
>> + SBufList::const_iterator i = list.begin();
>> + rv.append(*i);
>> + ++i;
>> + for (; i!= list.end(); ++i) {
>> + rv.append(separator);
>> + rv.append(*i);
>> + }
>
> Consider using std::accumulate with an appropriate "operator" class
> instead of the above. See a sketch below.
>
> There is also an empty comment at the start of that block.
Doh. Added comment.
>> + sz += separator.length() * list.size();
>
> Not a big deal, but this is wrong for single-item lists that do not use
> a separator.
It's only an optimization, worst case the resulting SBuf will have one
separator.length() extra backing storage. As you already mentioned,
the elephant there is list.size() anyway..
>> + rv.rawSpace(sz);
>
> Please use reserveSpace() if you do not need to access the reserved
> space immediately.
Ok.
>>>> + SBufList::const_iterator i;
>>>> + // calculate the total size of the return value
>>>> + SBuf::size_type sz = 0;
>>>> + for (i = list.begin(); i != list.end(); ++i)
>>>> + sz += i->length();
>>>> + sz += separator.length()*list.size();
>>>> +
>>>> + SBuf rv;
>>>> + rv.rawSpace(sz);
>>>> +
>>>> + i = list.begin();
>>>> + rv.append(*i);
>>>> + ++i;
>>>> + for (; i!= list.end(); ++i) {
>>>> + rv.append(separator);
>>>> + rv.append(*i);
>>>> + }
>>>> + return rv;
>>>> +}
>>>
>>>
>>> Consider using std::accumulate with an appropriate operator instead of
>>> the above. Providing such an operator would allow folks to "join" SBufs
>>> stored in any SBUf container, not just SBufList.
>>
>> Size precalculation is an optimization. I've made use of
>> std:accumulate via SBufListAccumulateSizes, but I'm not sure it'd be
>> worthwile to make that available throughout the code base.
>
> My point was that joining SBuf containers is useful for all container
> types, but your code only works for std::lists. I think you can provide
> the same functionality, with the same level of optimization, by making
> the code generic. Here is a sketch:
>
> {
> SBuf::size_type mergedSize = 0;
> std::accumulate(..., mergedSize, AddSize(separator));
>
> SBuf merged;
> merged.reserveSpace(mergedSize);
> std::accumulate(..., merged, AddContent(separator));
>
> return merged;
> }
>
> The missing "..." parts just contain items.begin(), items.end(), where
> bufs is the container.
Ok.
Let's try to sketch the grand plan, shall we?
What about using a SBufAlgs.h which contains
SBufEqual
SBufStartsWith
SBufAddLength(separator=SBuf())
template<class container> SBufContainerJoin<container>(...)
plus a specialized SBufListJoin which is simply an instantiation of
SBufContainerJoin<SBufList>?
This should cover it all.
Thanks,
/kinkie
