[squid-dev] [PATCH] improve CharacterSet

[Kinkie]

Hi all,
  the code is polished and ready to be merged. unfortunately I hit a
snag while trying to actually make use of it: helpers can rely on code
in lib/, but not in src, due to the order subdirectories are built.

I've thus changed order of subdirs in the top-level Makefile.am so
that src/ is built before helpers; we have a similar dependency for
tools/, we need to remember this.

Polished and merged as r14472. Thanks!

On Sat, Jan 2, 2016 at 1:40 AM, Amos Jeffries <squid3 at treenet.co.nz> wrote:
> On 2015-12-31 07:08, Alex Rousskov wrote:
>>
>> On 12/30/2015 10:05 AM, Kinkie wrote:
>>>
>>> Sure. Are you +1'ing the patch (with the change)?
>>
>>
>> In general, I hesitate +1ing patches that I have not closely reviewed. I
>> have not verified that QDTEXT initialization is correct, for example.
>> Please note that your own [implied] +1 should be enough if you are
>> confident that this is the right change.
>>
>>
>> Besides the inlining issue, you have some inconsistent NULL/nullptr use
>> [in test cases]. If the proposed commit message does not disclose
>> addition of the previously commented-out sets, please amend it.
>
>
> * since this patch is described as "improving CharacterSet" and does API
> polishings I expect the NULL->nullptr conversion is in scope for this
> small(ish) part of the code. Both in the new and old code, and comments.
>
> * the operator+() comment still says "src" where the param is now "rhs".
>
> * the comment is doxygen associated with == but not !=, and is somewhat
> redundant.
>  - I think "///< \note ignores label" after each define should be
> sufficient.
>
> * the comments on the new .h operator +/-/<< definitions should be using
> \return etc.
>
>
> Other than that it looks fine to me. So..
>
> +1 with the above extra polishing.
>
> Amos
>
>
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev



-- 
    Francesco