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