Last week-end, working on refactoring existing CAD Exchanger ACIS converter code (to maximize its reusability for the Parasolid one which is under development), I encountered an issue which might be interesting for a broader audience.
Inside the CAD Exchanger SDK, there is a function that accepts a pointer to the object (Base_Generator*) that inherits Standard_Transient (i.e. which can be manipulated by handle).
DEFINE_STANDARD_HANDLE(Base_Generator,Standard_Transient)
class Base_Generator : public Standard_Transient
{
...
};
class Base_GDriver
{
...
//! Translates an object into a result.
virtual void Paste (const Source& theSource,
Target& theTarget,
Base_Generator* theGenerator) const = 0;
};
I don’t remember the fundamental reason why the pointer was originally preferred to a const reference (i.e. const Handle(Base_Generator)&) , likely becase to avoid splitting Base_Generator.hxx into two headers – one defining the Base_Generator class and the other – Handle_Base_Generator, as Base_Generator.hxx itself included the header defining Base_GDriver. That does not matter much in this context anyway.
Inside the upper-level code which creates an instance of Base_Generator subclass (ACISGTopo_RGenerator), I decided to avoid using a handle and allocating a dynamic memory and simply created it on stack. That is, instead of:
Handle(ACISGTopo_RGenerator) aRGenerator = new ACISGTopo_RGenerator();
I simply use:
ACISGTopo_RGenerator aRGenerator;
This is fine as the object is not used outside the scope and no smart pointer is required.
However, when I started regression testing, I have encountered an access violation exception. Debugging led to a code that has been modified during the refactoring:
void ACISGGeom_BaseIntCurDriver::Paste (
const Handle(Standard_Transient)& theSource,
Handle(Standard_Transient)& theTarget,
Base_Generator* theGenerator) const
{
...
const Standard_Real aTol =
Handle(ACISGTopo_RGenerator)::DownCast (theGenerator)->Model()->Header()->ResAbs();
...
}
Anyone sees the issue now ?
That should be obvious if you have dealt with handles quite enough. Here is the explanation. During DownCast() a temporary handle object created. During its creation it increments the reference counter (which is 0 by default in aRGenerator). Upon its destruction it decrements it back to 0 and the handle’s destructor calls the object destructor thereby destroying the aRGenerator and deallocating its memory. That’s it!
I ended up changing the ACISGGeom_BaseIntCurDriver::Paste() to use static cast:
const Standard_Real aTol = static_cast(theGenerator)->Model()->Header()->ResAbs();
as this is a guaranteed cast in this particular case.
In general, the most reliable approach would be to wrap aRGenerator with a handle (instead of creating it on a stack).
So, a few closing thoughts:
1. Test, test, test !
2. Even if you understand bolts and nuts of the handle, you can still be surprised;
3. If you have to supply a pointer to your object that inherits Standard_Transient into a third-party code, make sure your always wrap it with a Handle;
4. If the Handle_ constructors that accepts a pointer to an object (e.g. Handle_Standard_Transient (const Standard_Transient*) ) were declared with ‘explicit’ keyword then the compiler would fail at DownCast() and this could be a hint to a developer to think carefully what might happen. Perhaps a candidate for a next modification of the OCC source code ?
Take care!
Inside the CAD Exchanger SDK, there is a function that accepts a pointer to the object (Base_Generator*) that inherits Standard_Transient (i.e. which can be manipulated by handle).
DEFINE_STANDARD_HANDLE(Base_Generator,Standard_Transient)
class Base_Generator : public Standard_Transient
{
...
};
class Base_GDriver
{
...
//! Translates an object into a result.
virtual void Paste (const Source& theSource,
Target& theTarget,
Base_Generator* theGenerator) const = 0;
};
I don’t remember the fundamental reason why the pointer was originally preferred to a const reference (i.e. const Handle(Base_Generator)&) , likely becase to avoid splitting Base_Generator.hxx into two headers – one defining the Base_Generator class and the other – Handle_Base_Generator, as Base_Generator.hxx itself included the header defining Base_GDriver. That does not matter much in this context anyway.
Inside the upper-level code which creates an instance of Base_Generator subclass (ACISGTopo_RGenerator), I decided to avoid using a handle and allocating a dynamic memory and simply created it on stack. That is, instead of:
Handle(ACISGTopo_RGenerator) aRGenerator = new ACISGTopo_RGenerator();
I simply use:
ACISGTopo_RGenerator aRGenerator;
This is fine as the object is not used outside the scope and no smart pointer is required.
However, when I started regression testing, I have encountered an access violation exception. Debugging led to a code that has been modified during the refactoring:
void ACISGGeom_BaseIntCurDriver::Paste (
const Handle(Standard_Transient)& theSource,
Handle(Standard_Transient)& theTarget,
Base_Generator* theGenerator) const
{
...
const Standard_Real aTol =
Handle(ACISGTopo_RGenerator)::DownCast (theGenerator)->Model()->Header()->ResAbs();
...
}
Anyone sees the issue now ?
That should be obvious if you have dealt with handles quite enough. Here is the explanation. During DownCast() a temporary handle object created. During its creation it increments the reference counter (which is 0 by default in aRGenerator). Upon its destruction it decrements it back to 0 and the handle’s destructor calls the object destructor thereby destroying the aRGenerator and deallocating its memory. That’s it!
I ended up changing the ACISGGeom_BaseIntCurDriver::Paste() to use static cast:
const Standard_Real aTol = static_cast
as this is a guaranteed cast in this particular case.
In general, the most reliable approach would be to wrap aRGenerator with a handle (instead of creating it on a stack).
So, a few closing thoughts:
1. Test, test, test !
2. Even if you understand bolts and nuts of the handle, you can still be surprised;
3. If you have to supply a pointer to your object that inherits Standard_Transient into a third-party code, make sure your always wrap it with a Handle;
4. If the Handle_ constructors that accepts a pointer to an object (e.g. Handle_Standard_Transient (const Standard_Transient*) ) were declared with ‘explicit’ keyword then the compiler would fail at DownCast() and this could be a hint to a developer to think carefully what might happen. Perhaps a candidate for a next modification of the OCC source code ?
Take care!