[squid-dev] [PATCH] Replace new/delete operators using modern C++ rules

[Kinkie]

Hi,
  I can check Mac.
We have had a donation of Solaris nodes, though I need to revise and
unrot the setup. I'll do that as soon as possible but don't hold your
breath.

On Sun, Apr 10, 2016 at 5:50 AM, Amos Jeffries <squid3 at treenet.co.nz> wrote:
> On 7/04/2016 7:05 p.m., Alex Rousskov wrote:
>> Hello,
>>
>>     This change was motivated by "Mismatched free()/delete/delete[]"
>> errors reported by valgrind and mused about in Squid source code.
>>
>> I speculate that the old new/delete replacement code was the result of
>> slow accumulation of working hacks to accomodate various environments,
>> as compiler support for the feature evolved. The cumulative result does
>> not actually work well (see the above paragraph), and the replacement
>> functions had the following visible coding problems according to [1,2]:
>>
>> a) Declared with non-standard profiles that included throw specifiers.
>>
>> b) Declared inline. C++ says that the results of inline declarations
>>    have unspecified effects. In Squid, they probably necessitated
>>    complex compiler-specific "extern inline" workarounds.
>>
>> c) Defined in the header file. C++ says that defining replacements "in
>>    any source file" is enough and that multiple replacements per
>>    program (which is what a header file definition produces) result in
>>    "undefined behavior".
>>
>> d) Declared inconsistently (only 2 out of 4 flavors). Declaring one base
>>    flavor should be sufficient, but if we declare more, we should
>>    declare all of them.
>>
>> [1] http://en.cppreference.com/w/cpp/memory/new/operator_new
>> [2] http://en.cppreference.com/w/cpp/memory/new/operator_delete
>>
>> The replacements were not provided to clang (trunk r13219), but there
>> was no explanation why. This patch does not change that exclusion.
>>
>>
>> I have no idea whether any of the old hacks are still necessary in some
>> cases. However, I suspect that either we do not care much if the
>> replacements are not enabled on some poorly supported platforms OR we
>> can disable them (or make them work) using much simpler hacks for the
>> platforms we do care about.
>>
>>
>> If the patch is approved, the SquidNew.h file should be removed. The
>> patch does not contain this change, but I hope that removing that file
>> should not be difficult -- it is not even mentioned in Makefile.am files
>> AFAICT.
>>
>> The attached patch does not remove Debug::OutStream class that was
>> created to work around some of the above problems. There is a different
>> pending patch ("Do not hide important/critical messages") which also
>> removes that class completely (for somewhat different reasons):
>> http://lists.squid-cache.org/pipermail/squid-dev/2016-March/005510.html
>>
>
> AFAIK we should still have MacOS and Solaris people around to check that
> this update does not break anything for them. Though the BuildFarm does
> not have nodes to do it ourselves.
>
> In principle this is a great step forward, but I would like confirmation
> about the portability side of things before it actually gets merged.
>
> Amos
>
>
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev



-- 
    Francesco