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