[squid-dev] [PATCH] improve CharacterSet

[Alex Rousskov]

On 01/02/2016 06:52 AM, Kinkie wrote:

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

This build order change should not be a part of the "improve
CharacterSet" patch because it has nothing to do with improving
CharacterSet API. Whether helpers are allowed to use src/ is a separate
question, implicitly being discussed on the "SBuf API" thread right now.

Alex.


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