<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 20, 2015 at 7:11 AM, Amos Jeffries <span dir="ltr"><<a href="mailto:squid3@treenet.co.nz" target="_blank">squid3@treenet.co.nz</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 20/08/2015 7:28 a.m., Kinkie wrote:<br>
> Hi all,<br>
>   the attached patch (a merge request from<br>
> lp:~kinkie/squid/mempools-nozero) changes MEMPROXY_CLASS so that it doesn't<br>
> zero memory before releasing it to requestors. The constructors of all<br>
> classes using that feature were checked, so that they fully initialize all<br>
> data members and thus require no zeroing.<br>
> It also changes Debug::OutStream to use MEMPROXY_CLASS instead of<br>
> new/derelete, and changes constructors to initializer lists whenever<br>
> possible and sensible (aka justified by other changes), same for dropping<br>
> HERE and adopting nullptr.<br>
> It also changes some code in Makefile.am to use make variables in place of<br>
> full paths.<br>
> The code has been run-tested (default config only), build-farm-tested and<br>
> polygraph-tested. It shows a small but noticeable performance improvement<br>
> in performance (of course, numbers are to be taken with a big grain of<br>
> salt).<br>
><br>
> When reviewing, please do not focus on formatting issues; if this patch<br>
> passes review I'll run a source-formatting before merging.<br>
<br>
<br>
</span>* in Makefiles please add the new bits of *_SOURCE lists in alphabetical<br>
order.<br>
 - you have several stub_libmem.cc going after stub_M and stub_S* filenames.<br></blockquote><div><br></div><div>I think I fixed them. It was mentioned in the coverity-fixes thread that it could be a good idea to do a formatting round sorting all entries in Makefiles, once the updated standards are finalized. I'd defer any leftover unsorted entries till then.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* Please make the initializer lists one member per line.<br>
 - the auto-formatting wont do that for us yet.<br>
 - it greatly helps readability and manual checking for missing or<br>
out-of-order items. We can also then easily add comments to lines<br>
explain odd default values.<br></blockquote><div><br></div><div>Sure for .cc, not so sure for .h, where compactness helps readability and presence and order should be easier to spot. Finally, many missing or out-of-order should get caught either by Coverity or gcc.<br></div><div>Changed for all .cc, not done for .h (where lists are quite shorter)<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* in memset() please use 0 integer value not '\0' to set flags.<br>
 - it helps the compiler know it is safe to optimize up to larger than<br>
8-bit values of 0.<br></blockquote><div><br></div><div>Ok.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* in MemObject::MemObject() please leave the _reply initialization like<br>
it was so that the HTTMSGLOCK hack is kept visibly associated with the<br>
new operation.<br></blockquote><div><br></div><div>Ok.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* You add several "// needs mempool-managed zeroing."<br>
 - but you say this patch makes the MEMPROXYC_CLASS macro do zero-ing<br>
already. Those should be reverted.<br></blockquote><div><br></div><div>The commets are wrong and based on an outdated analysis of StoreMeta. Removed the comments.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* src/HttpHeader.cc - revert<br></blockquote><div><br></div><div>Yes. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* when adding a new constructor ensure the big-5 operators (ctor, dtor,<br>
assign, emplace, emplace-assign) are all present. Even if they use "=<br>
default"<br>
 - looking at StoreMeta, maybe others.<br></blockquote><div><br></div><div>StoreMeta is pure-virtual; Also, it has no complex members so emplace doesn't add value vs. copy.<br></div><div>Omitting emplace and implementing the others as protected for completeness' sake.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* wordlist missing 'explicit' on the raw-pointer ctor<br>
 - also other big-5 operators<br></blockquote><div><br></div><div>wordlist must die; it's a C relic which doesn't belong to squid anymore.<br></div><div>Kinda-implemented with a private default destructor (friended by wordlistDestroy) and deleted assignment and copyconstructor. This helped catch a leak in FtpGateway.cc so at least it was used for something -even if it's very ugly.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* in test-suite/Makefile.am please try to avoid adding LIBMEM_STUBS.<br>
- DEBUG_SOURCE entries should be enough, and it doesnt matter of extra<br>
stubs get linked to unit tests.<br></blockquote><div><br></div><div>Done in a different way: some unit tests need the full <a href="http://libmem.la">libmem.la</a>; the stub symbols would override the library, causing failures.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* please do one of these:<br>
<br>
 + return the XXX to Debugs.h, but it can move up to explain why<br>
AllocatorProxy.h is used instead of mem/forward.h<br></blockquote><div><br></div><div>It doesn't need to. I'm dumb and should have used mem/forward in the first place - you had even told me not too long ago.<br></div><div><br></div><div>Revised patch (with huge context, hence the big size) attached.<br><br></div><div>Thanks<br clear="all"></div></div><br>-- <br><div class="gmail_signature">    Francesco</div>
</div></div>