[Insight-developers] important dashboard warning...

[Luis Ibanez]

Hi David,


Thanks for pointing this out.


Probably the right solution is to change the code in
itkMatrixOffsetTransformBase to have the GetVarInverseMatrix()
return a const reference to the inverse matrix.


As you suggested, something like:



cvs diff: Diffing .
Index: itkMatrixOffsetTransformBase.h
===================================================================
RCS file: 
/cvsroot/Insight/Insight/Code/Common/itkMatrixOffsetTransformBase.h,v
retrieving revision 1.15
diff -r1.15 itkMatrixOffsetTransformBase.h
365c365
<   InverseMatrixType GetVarInverseMatrix( void ) const
---
 >   const InverseMatrixType & GetVarInverseMatrix( void ) const

====================================================================



I'm running an experimental build with this change.
If it looks good, the change will be committed later today.



   Luis



-----------------------------------
David Cole wrote:
> Hello ITK developers,
> 
> This warning:
> /.../Insight/Code/Common/itkQuaternionRigidTransform.txx: In member
> function `const typename
> itk::QuaternionRigidTransform<TScalarType>::InverseMatrixType&
> itk::QuaternionRigidTransform<TScalarType>::GetInverseMatrix() const
> [with TScalarType = double]':
> /.../DEBUG_64_64/Insight/Code/Common/Templates/itkQuaternionRigidTransformD.cxx:4: 
> 
>  instantiated from here
> /.../Insight/Code/Common/itkQuaternionRigidTransform.txx:220: warning:
> returning reference to temporary
> 
> (found here: 
> http://www.itk.org/Testing/Sites/chiarugi.uiowa/Linux64-g++-3.4-dbg/20070329-0100-Nightly/BuildWarning.html 
> 
> )
> 
> ...indicates a "bad thing". It's an important warning to fix by
> changing the code (as opposed to ignoring it or masking the warning
> from dashboard results). The method
> QuaternionRigidTransform::GetInverseMatrix is declared like so:
>  const InverseMatrixType & GetInverseMatrix( void ) const;
> in the protected section of QuaternionRigidTransform.
> 
> In the implementation of that method, however, the return statement is:
>  return this->GetVarInverseMatrix();
> 
> ...but GetVarInverseMatrix is defined like this:
>  InverseMatrixType GetVarInverseMatrix( void ) const
> 
> Just like you shouldn't return a local variable by reference, neither
> should you return a temporary variable by reference. The copy only
> exists as long as that temporary is in scope, so the caller may not
> actually be referencing a valid object when he goes to use it. The
> caller's reference variable is given a reference to something that may
> go out of scope prior to the reference variable itself going out of
> scope......
> 
> So my questions for the group are these:
> Who's the right person to fix this?
> What's the right fix?
> Is it ok to change the protected method GetVarInverseMatrix so that it
> returns a "const&" rather than a copy of the data member?
> And if we did that, is that the right thing to do in this case?
> 
> Thanks for wading through this email if you've made it this far...
> 
> David Cole
> Kitware, Inc.
> _______________________________________________
> Insight-developers mailing list
> Insight-developers at itk.org
> http://www.itk.org/mailman/listinfo/insight-developers
>