Re: [PATCH] Make SBuf::find_first_of O(N) and implement find_first_not_of

On 12/17/2013 10:35 AM, Kinkie wrote:

> here's the updated patch.

Looks good to me, thank you. There is only one bug (listed first below).
The rest is minor polishing:

> + std::copy_if(src.chars_.begin(),src.chars_.end(),chars_.begin(),isNonZero);

copy_if does not advance the output iterator when the predicate returns
false, so you will be sequentially populating chars_ with 1s in src,
without any gaps. Care to add a += test case to catch such bugs?

BTW, std::copy_if is a C++11 feature. Are you sure it is a good idea to
use it for Squid core functionality today?

> +#include "squid.h"
> +
> +#include "CharacterSet.h"

Extra empty line?

> + typedef std::vector<uint8_t> vector_type;

Since character storage does not have to be a vector, consider
s/vector_type/Characters/ or s/vector_type/Storage/.

> /// define a character set with the given label ("anonymous" if NULL,

s/,/)/

> + bool operator[](unsigned char c) const {return chars_[static_cast<uint8_t>(c)] == 1;}

I would remove the " == 1" part to make the code simpler, avoid
repeating magic constants, make it consistent with isNonZero() if that
stays, and possibly make the code faster (not sure whether non-zero test
is a tad faster than an equality test).

> + /// add a given character to the character set.

Please remove '.' since you are not starting with a capital letter and
for consistency sake. You can also polish other new comments with
similar criteria in mind if you want.

> + /** characters present in this set.

Consider "An index of characters in the set." instead. We do not really
store characters themselves.

All of the above can be addressed at commit time -- no need for another
round of reviews as far as I am concerned.

Thank you,

Alex.

> On Tue, Dec 17, 2013 at 6:04 PM, Kinkie <gkinkie_at_gmail.com> wrote:
>> On Tue, Dec 17, 2013 at 6:06 AM, Alex Rousskov
>> <rousskov_at_measurement-factory.com> wrote:
>>> On 12/16/2013 03:36 PM, Alex Rousskov wrote:
>>>>> Rewrite the += operator loop to simply
>>>>> this->add() every character in the src set. Use std::for_each or another
>>>>> <algorithm> for that if possible.
>>>
>>>> I think the above is still valid though.
>>>
>>> but I cannot find a suitable algorithm. If you cannot either, just use
>>> add() calls in an explicit loop.
>>
>> std::copy_if is perfect for the job; using that.
>>
>> --
>> /kinkie
>
>
>
Received on Tue Dec 17 2013 - 19:10:35 MST