On 17 Dec 2013, at 20:09, Alex Rousskov <rouss...@measurement-factory.com>
wrote:
> 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?
Ok, I'll revert to the previous implementation. Sorry for overlooking both
facts.
>> +#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).
I expect the compiler to optimise that away; it's meant to be an explicit cast
to bool. To avoid magic constants and make it similar to C, I could rework it
as != 0.
>> + /// 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.
Ok, thanks for taking the time.
I'll commit as soon as trunk is stable again, then immediately follow up to a
camelCase-izatiion of SBuf::find_first{,_not}_of.
Kinkie
