On 06/28/2016 08:52 AM, Amos Jeffries wrote: > On 28/06/2016 7:36 a.m., Alex Rousskov wrote: >> 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? > So that LockingPointer is not affected by the unique_ptr change. That does not compute for me: Clearly, LockingPointer _is_ affected by the unique_ptr change in your patch. And if you meant that LockingPointer _callers_ should not be affected, then using unique_ptr for LockingPointer _implementation_ will not affect LockingPointer callers AFAICT. > Without that LockingPointer would inherit from std::unique_ptr. I hope you can avoid duplication without making unique_ptr a LockingPointer parent. You can use unique_ptr for the LockingPointer data member. >> Can we [continue to] use unique_ptr in LockingPointer >> instead, now as a data member? > I found we cannot. LockingPointer is doing too much non-unique_ptr > things with the locks. It is like a demented cross between unique_ptr, > shared_ptr and cbdata. This does not compute for me: You have claimed that TidyPointer, the former LockingPointer parent, is essentially a unique_ptr. I agreed with that claim. Now you are claiming that you cannot use unique_ptr as a private data member of LockingPointer because LockingPointer does unusual things. Since you are not changing what LockingPointer does, I would expect LockingPointer to [continue to] be able to use TidyPointer/unique_ptr, regardless of how unusual LockingPointer is. Both claims cannot be true at the same time AFAICT... To make progress, I will rephrase the question: What unique_ptr properties prevent you from using it for LockingPointer::raw? > Thats also why I wanted to pause here and get an audit. You did the right thing. >> 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. > Nod. Its not exactly duplication though. It was cut-n-paste, not > copy-n-paste. So if anything is duplicated it was beforehand too, just > not so obviousy so. TidyPointer was certainly duplicating unique_ptr. IIRC, there was a valid reason for that functionality duplication -- Squid did not have a reliable access to unique_ptr when TidyPointer was written. Note that there was no code duplication from Squid point of view; there was a "reinventing the wheel" problem instead. Now, when replacing TidyPointer with the now-available std::unique_ptr class, you can no longer use unique_ptr unavailability as an excuse to duplicate TidyPointer/unique_ptr functionality in the LockingPointer. >>> 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. > Thats how I took the documentation, in fact it says so outright in > place. But I was not able to easily get it to accept either a function > pointer, or a function type. Yeah. I think there are two different problems here, and that obscures the solution. I will illustrate using BIGNUM_Pointer as an example: A) The Deleter template parameter must be a type, not an object. Thus, declaring BIGNUM_Pointer using BN_free_cpp as Deleter does not work: BN_free_cpp is not a type but an object. B) We do not want to specify the Deleter object explicitly every time we are defining our BIGNUM_Pointer objects. Thus, our Deleter type must have a default constructor. Pointers to functions do not have default constructors. Thus, the Deleter type must be a [DefaultConstructible] class and not just a "pointer to function X" type. All of the following "typedef ... BIGNUM_Pointer" variants compile in my tests: 1. std::unique_ptr<BIGNUM, std::function<decltype(BN_free_cpp)> > 2. std::unique_ptr<BIGNUM, std::function<decltype(BN_free)> > 3a. std::unique_ptr<BIGNUM, decltype(&BN_free_cpp) > 3b. std::unique_ptr<BIGNUM, void(*)(BIGNUM*) > #3a and #3b force you to pass a specific deleter when defining BIGNUM_Pointer objects, violating condition (B) above. For example: BIGNUM_Pointer bnp(..., &BN_free_cpp); #2 might suffer from the same problem that forced us to add the CtoCpp1 macro. Christos, the author of r11100 that added CtoCpp1, may be able to confirm or deny this. > ../../../src/ssl/gadgets.h:51:44: note: expected a type, got > ‘Ssl::BN_free_cpp’ Yes, this is due to (A) above. Solutions #1-3 avoid that problem. > If I give it the type instead of function pointer, like so: > typedef std::unique_ptr<BIGNUM, decltype(BN_free_cpp)> BIGNUM_Pointer; AFAICT, you want a reference to the function, not the function name itself. I do not know why the usual decaying rules do not apply here, but perhaps that is not important. #3a solves this but violates (B) as discussed above. > Asside from that the functor keeps the code looking almost the same as > it did before. So a bit less pain for all of us porting things to 3.5 in > the next years. I hope the above both explains exactly why we need a functor here and offers a similar std::function-based solution that does not require re-inventing UnaryFunctor. Thank you, Alex. _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
