[squid-dev] [PATCH] TidyPointer removal

[Alex Rousskov]

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.