Created attachment 104 [details]
automate memory management of RandomVariable

It would be possible to Ptr<RandomVariable> too but I decided against this because it would have broken the user-visible API where you can pass around a UniformVariable class by value to classes which need a RandomVariable.

(In reply to comment #1)
> Created an attachment (id=104) [edit]
> automate memory management of RandomVariable
> 

Mathieu, could you provide a summary of this patch?  From what I see, it appears that RandomVariables now are passed only by value.  It also appears that there is some renaming and some extra indirection introduced into the object and ownership heirarchy (RandomVariable owns a RandomVariableBase, which is subclassed by Impl classes?)

(In reply to comment #3)

> Mathieu, could you provide a summary of this patch?  From what I see, it
> appears that RandomVariables now are passed only by value.  It also appears

yes.

> that there is some renaming and some extra indirection introduced into the
> object and ownership heirarchy (RandomVariable owns a RandomVariableBase, which
> is subclassed by Impl classes?)

Yes. Details below.

The previously-existing RandomVariable base class has been renamed to RandomVariableBase. The previously-existing XXXVariable classes have been renamed to XXXVariableImpl and they now subclass RandomVariableBase. Finally, a new RandomVariable base class has been introduced which holds a pointer to a RandomVariableBase subclass and does the memory management of the underlying RandomVariableBase subclass. Basically, the RandomVariable class is not a smart pointer which knows how to copy and delete RandomVariableBase subclasses. The other XXXRandomVariable subclasses of the RandomVariable base class are just conveniant constructors and were added to avoid changing the user-visible API. i.e., to make it still possible to pass a temporary UniformVariable through a function call.

This patchset is basically replacing the existing C++ variables of type "RandomVariable *" into variables of type "RandomVariable". i.e., it is removing the pointer and relieves the user from doing memory management himself. As I said earlier, it would be trivially possible to achieve the same result by using Ptr<RandomVariable> but this would break the user-visible API since the user would not be able anymore to pass around temporary UniformVariable objects.

(In reply to comment #4)
> RandomVariableBase subclass. Basically, the RandomVariable class is not a smart

This should have read:

Basically, the RandomVariable class is _now_ a smart point [...]

Just to be clear on declaration semantics, I now do the following to get different distributions:

RandomVariable x = UniformVariableImpl(); 
RandomVariable y(ParetoVariableImpl());
//etc

Correct?  My only issue then is the fact that user code may have to include classes with the word "Impl" in the name which is scary.  Perhaps the RandomVariableBase subclasses could leave off the Impl naming?

Other than this clarification, I wouldn't be opposed to having this merged for this month's release.

(In reply to comment #6)
> Just to be clear on declaration semantics, I now do the following to get
> different distributions:
> 
> RandomVariable x = UniformVariableImpl(); 
> RandomVariable y(ParetoVariableImpl());
> //etc
> 
> Correct?  My only issue then is the fact that user code may have to include

nope :)

> classes with the word "Impl" in the name which is scary.  Perhaps the
> RandomVariableBase subclasses could leave off the Impl naming?

The user code stays the same and does this:

RandomVariable x = UniformVariable ();

The UniformVariable class constructor calls RandomVariable (new UniformVariableImpl ()) but this is done behind the scene on behalf of the user.

> Other than this clarification, I wouldn't be opposed to having this merged for
> this month's release.

What you suggest could be used to achieve the same syntax I think but I have not tried to implement it. Do you see this as a more desirable solution and do you want me to attempt to implement it ?

(In reply to comment #7)
[snip]
> What you suggest could be used to achieve the same syntax I think but I have
> not tried to implement it. Do you see this as a more desirable solution and do
> you want me to attempt to implement it ?

No, I am fine with what you've posted.

changeset:   28ce210b91bb