[squid-dev] [PATCH] TidyPointer removal

[Alex Rousskov]

On 06/27/2016 04:35 AM, Amos Jeffries wrote:
> This splits TidyPointer and LockingPointer by removing the inheritence
> and copying the needed TidyPointer code into the LockingPointer as-is.

Why start duplicating TidyPointer/unique_ptr functionality in
LockingPointer? Can we [continue to] use unique_ptr in LockingPointer
instead, now as a data member?


> The resulting situation is that TidyPointer is a stand-alone Pointer
> type that duplicates the std::unique_ptr API so closely it can almost be
> a drop-in replacement. The remaining difference is that TidyPointer took
> a function pointer whereas unique_ptr uses a Functor type.
> 
> Adding a UniaryFunctor(function, arg) macro in src/base/MakeFunctor.h to
> replace the CtoCpp1(function, arg) macro used by TidyPointer allows us
> to remove TidyPointer completely from Squid-4.

Replacing custom TidyPointer with standard unique_ptr is a welcomed
improvement that can and should be accepted. Whether you commit those
[polished] changes before or while fixing "LockingPointer issues" is
your call. However, this reasoning alone does not make code duplication
necessary or acceptable.


> The LockingPointer still has the issues which led us down the path to
> getting here. I've chosen to stop and submit for review at this point to
> ensure we still have a fully working Squid before going further down
> this trail.

Stopping here is perfectly fine, but please do not duplicate
TidyPointer/unique_ptr code/functionality. That duplication did not
exist before, so you cannot argue that it is one of the old
LockingPointer issues that you will fix later.


> unique_ptr uses a Functor type.

This statement is inaccurate in its context -- unique_ptr does accept
[lvalue references to] functions as well. I believe this is by design --
any good template should not care whether it was given a simple function
or a Functor object.


> +/// DeAllocator functor for pointers that need free(3) from the std C library
> +UniaryFunctor(xfree, char *);

Where is xfree_functor used? I only see xfree_char() which is defined
without using UniaryFunctor.


> +// Macro to be used to define a C++ functor of an extern "C"
> +// function.

I do not see anything in this macro that would make it specific to
extern "C" functions. I think it works with any function. The old
CtoCpp1() macro was needed to convert "extern C" functions to "extern
C++" functions; the [ugly] macro description from r11100 "worked"
because the macro changed only one aspect -- the linking style changed
from C to C++. Here, you are changing two aspects at once, resulting in
a far less helpful comment IMO.


Combined, these observations related to adding functors lead me to
suspect that we do not need to replace CtoCpp1 with UniaryFunctor. What
errors do you get when building with CtoCpp1 instead of UniaryFunctor?

Similarly, is there something wrong with tidyFree() that forces you to
add xfree_char() [with a botched description]?


> +#define UniaryFunctor(function, argument_type) \

s/Uniary/Unary/ to fix spelling


I hope to post more polishing comments as well, but the code duplication
and functor issues should probably be resolved first.


Thank you,

Alex.
P.S. If you can post larger context diffs (e.g., -U30), please do so,
but there is no need to repost the last patch.