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

[Niels Dekker]

Hi Bradley,

Thank you for your encouraging feedback  :-)  Please have a look at our 
proxy based fix: "COMP: Fixed <nifti1_io.h> leak into user code", 
http://review.source.kitware.com/#/c/22461/


Kind regards, Niels

On 2017-06-22 16:58, Lowekamp, Bradley (NIH/NLM/LHC) [C] wrote:
> Hello,
> 
> I looked into this a little the other day. I tried #2, but it did not
> work due to the way `nifty_image` is a typedef an anonymous structure.
> 
> Your #3 approach looks good to me. I look forward to seeing the patch.
> 
> You can also make ITKNIFTI a private dependency for the module by
> undoing the following:
> https://github.com/InsightSoftwareConsortium/ITK/commit/67e308fc8470c8119323a8954d53c777966c5ad5
> 
> 
> On 6/22/17, 10:18 AM, "Niels Dekker" <niels-xtk at xs4all.nl> wrote:
> 
>     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