<p dir="ltr">My point.<br>
At the same time, I'd like others to chime in as well. It is a tiny change (turn an assert into an if) but the implications are quite profound.<br>
I don't expect it'll fault right away - the assert is not firing right now so clients obviously take care of checking.</p>
<div class="gmail_quote">On Sep 29, 2015 11:34 AM, "Amos Jeffries" <<a href="mailto:squid3@treenet.co.nz">squid3@treenet.co.nz</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 29/09/2015 5:59 p.m., Kinkie wrote:<br>
> On Mon, Sep 28, 2015 at 10:10 AM, Amos Jeffries <<a href="mailto:squid3@treenet.co.nz">squid3@treenet.co.nz</a>> wrote:<br>
>> On 28/09/2015 7:28 p.m., Kinkie wrote:<br>
>>> Whoops, sorry.<br>
>>> The patch includes spurious changes.<br>
>>> Updated patch attached.<br>
>>><br>
>><br>
>> +1.  Though please fix the indentation on "delete d;" in chunk @1100.<br>
>> Which should be 4 not 8 SP.<br>
><br>
> Huh?<br>
> code is:<br>
>     if (d)<br>
>         delete d;<br>
><br>
> Indentation looks legitimate to me..<br>
><br>
> BTW: there is a substantial difference in the memproxy_class behavior<br>
> wrt standard "delete": AFAICS, "delete nullptr" is a perfectly<br>
> legitimate statement by the standard and it is a NOP. When going<br>
> through mempools, it reaches MemImplementingAllocator::freeOne which<br>
> asserts. would it make sense to NOP ourselves?<br>
><br>
<br>
Yes. That is a bug.<br>
<br>
delete should always be the same API semantics and promises.<br>
<br>
So no need for the if(d) to exist around a delete, and if that causes<br>
crashes/errors inside our custom delete operators they are what is borked.<br>
<br>
Amos<br>
<br>
_______________________________________________<br>
squid-dev mailing list<br>
<a href="mailto:squid-dev@lists.squid-cache.org">squid-dev@lists.squid-cache.org</a><br>
<a href="http://lists.squid-cache.org/listinfo/squid-dev" rel="noreferrer" target="_blank">http://lists.squid-cache.org/listinfo/squid-dev</a><br>
</blockquote></div>