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. _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
