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

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

Thanks

>> + 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?

No. We probably want to be more aggressive (auto being the most
sought-after feature, but I'd add lambdas to the mix), but I'd venture
for a 2H2014 for that..

>> +#include "squid.h"
>> +
>> +#include "CharacterSet.h"
>
> Extra empty line?

Fixed.

>> + 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/.

Storage it is.

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

Ok

>> + 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).

As you tested in your other mail: I used the "!= 0" idiom as a way to
cleanly typecast to bool (I probably was not thinking straight when I
made the "similar to C" reference previously).

>> + /// 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.

Ok, I will do so as I cross such cases.

>
>
>> + /** characters present in this set.
>
> Consider "An index of characters in the set." instead. We do not really
> store characters themselves.

Done as "index of characters in the set"

> 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,

Thanks!
Received on Wed Dec 18 2013 - 17:31:18 MST