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
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
