[ITK-dev] #include <nifti1_io.h> leaking from itkNiftiImageIO.h into user code (+ 3 ways to fix it)

[Niels Dekker]

We're glad to see our niftilib-name-mangling patch made it into the 
master branch: 
https://github.com/InsightSoftwareConsortium/ITK/commit/370c3116d3edac3a3bc36e03e1b1b655df0f04af

Now we do have another nifty ITK issue! itkNiftiImageIO.h has an 
'#include <nifti1_io.h>', which leaks into user code:
https://github.com/InsightSoftwareConsortium/ITK/blob/61e8029ef53e2d37cfa3c5bad999e582326e36ee/Modules/IO/NIFTI/include/itkNiftiImageIO.h#L26
Our project uses both ITK and the NiftyReg library, and thereby 
indirectly uses two different versions of the niftilib library, with 
different versions of <nifti1_io.h>, so we foresee potential conflicts 
in the current situation. itkNiftiImageIO.h only needs <nifti1_io.h> for 
its 'nifti_image' typedef, as it has a private member, 
itk::NiftiImageIO::m_NiftiImage of type 'nifti_image *'. So basically 
<nifti1_io.h> is just an implementation detail of itk::NiftiImageIO. It 
is not part of the public interface of the ITK class.

Do you think it would be feasible to move '#include <nifti1_io.h>' from 
itkNiftiImageIO.h to its CXX file, itkNiftiImageIO.cxx?

We can think of three different ways to achieve this:

  1. The void pointer approach: Declare itk::NiftiImageIO::m_NiftiImage 
as a 'void *', instead of 'nifti_image *', and cast m_NiftiImage to 
'nifti_image *' within itkNiftiImageIO.cxx, whenever needed. I would 
expect more than 200 casts, doing static_cast< nifti_image 
*>(this->m_NiftiImage).

  2. The Pimpl way : Move the declaration of m_NiftiImage, and possibly 
any other data member of itk::NiftiImageIO, from itkNiftiImageIO.h to 
itkNiftiImageIO.cxx, according to the Pimpl idiom 
http://wiki.c2.com/?PimplIdiom  All access to these data members should 
then be adapted accordingly, so this would result in many modifications 
of existing code in itkNiftiImageIO.cxx. Optionally, private member 
function declarations could also be moved from itkNiftiImageIO.h to 
itkNiftiImageIO.cxx this way. It seems like a lot of work.

  3. The proxy approach: Declare itk::NiftiImageIO::m_NiftiImage as a 
reference to a proxy, which behaves like a 'nifti_image *'. This way, no 
existing lines of code from itkNiftiImageIO.cxx would need to be 
changed, except for the initialization of m_NiftiImage. The proxy class 
should be forward-declared in itkNiftiImageIO.h, and defined in 
itkNiftiImageIO.cxx, for example as follows:

	class NiftiImageProxy
	{
	  nifti_image* m_ptr;
	public:
	  NiftiImageProxy(nifti_image* ptr) ; // Converts pointer to proxy.
	  operator nifti_image*();  // Converts proxy to pointer.
	  nifti_image* operator->();
	};

We did fully implement and test the proxy approach, and we can make a 
patch, if you think that's an interesting option for ITK. What do you 
think?

Kind regards, also on behalf of my colleague Floris Berendsen,

     Niels
-- 
Niels Dekker
Scientific programmer
LKEB, Leiden University Medical Center, Netherlands