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

[Amos Jeffries]

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