Re: [PATCH] SBufList

On 5/12/2013 6:34 a.m., Kinkie wrote:
> Hi,
> attaching v4, hand-unrolling SBufListJoin (as before, missing
> Makefile.am changes), and marking wordlist as deprecated.
>
> On Tue, Dec 3, 2013 at 5:35 PM, Kinkie <gkinkie_at_gmail.com> wrote:
>>>> As this patch is a cherrypick of lp:~squid/squid/stringng, I'm not
>>>> extracting the Makefile.am changes as it's too time-consuming. These
>>>> changes are present in the branch Makefile.am and will be included at
>>>> the final merge time, but are not really significant for review, are
>>>> they?
>>>
>>> They are not, but a reviewer sometimes actually tests the patch. I know
>>> it sounds crazy, but it does happen once in a while. You have actually
>>> warned about the missing Makefile changes in your original submission,
>>> but I forgot that caveat after so many emails on the thread. Sorry!
>>
>> Crazy stuff, you're saying :D
>> The branch is routinely tested: I do test it before each submission,
>> and I will pass it by the entire build farm before merge. I wish not
>> to break the build as much as you do.
>>
>>> Since those exact Makefile changes would need to be done to trunk during
>>> commit, I am guessing you exclude them now to save time if the patch
>>> needs to be adjusted and re-posted for review, right? I am _not_ asking
>>> for those changes to be included in the patch. Just trying to understand
>>> your motivation or workflow. Not important.
>>
>> You perfectly got my motivation.
>>
>> This should be the last cherry picked off stringng, unless by chance
>> the API you are drafting for a tokenizer substantially overlap the
>> Tokenizer I have implemented in the stringng branch. If that happens,
>> I can try to adapt the current code to the new API. If it doesn't,
>> that code will be abandoned and the stringng branch closed.
>> I hope it will never happen again that a feature-branch lives as long
>> as this one has.
>>
>> --
>> /kinkie
>

in SBufAlgos.h:
* for all classes please put the first { of a class on a line of its own.

* inline is not necessary on methods defined within the class body. It
is for methods declared there and defined elsewhere in the .h or .cci

* SBufStartsWith::sensitive missing trailing _ as per squid coding
guidelines for private members.

* can SBufAddLength::operator()() be const ? or does that mess with the
STL requirements?

* please deflate:
+ SBuf::size_type sz;
+ sz =
 and make sz const.

in SBufList.cc
* I think we should avoid having the new code depend on or include the
old deprecated code. So things like ToSBufList(wordlist *wl) should be
added to wordlist.cc instead and removed when it is deleted.

in wordlist.h
* please ensure there is one empty line between a symbol declaration and
the doxygen comment about next symbol declared.

Amos
Received on Thu Dec 05 2013 - 11:18:43 MST