[squid-dev] [PATCH] improve CharacterSet

[Amos Jeffries]

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