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

[Alex Rousskov]

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


Thank you,

Alex.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: IP-12713-new-replacement-t3.patch
Type: text/x-diff
Size: 10677 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20160407/1184feac/attachment-0001.patch>