[Insight-developers] Small modification to ImportImageContainer
Luis Ibanez
luis.ibanez at kitware.com
Tue Mar 31 08:53:44 EDT 2009
Hi Tom,
Thanks for the clarification.
After looking closer at this two methods I see now why they can
both be const: They do not affect member variables (at least in
an obvious direct manner). This is still a bit deceptive, because
when DeallocateElements() is called, we pass a member variable as
its argument..., something that is also strange.... why is that
we simply not have the method without arguments, and internally
we access in it the member variable ?
It seems however that the argument of DeallocateElements can
be made non-const, and in that way you won't need to cast const
away.
Do you need this pointer argument to be const in the context
in which you will use this method in the derived class that you
are developing ?
I guess that we may understand better if you post the derived
class that you are implementing.
---
The ImportImageContainer class is at the very heart of ITK so we
should be very careful with any changes we made to it. :-/
Luis
------------
Tom Vercauteren wrote:
> (Sorry, mail continued)
>
> About const correctness. The AllocateElements() method was already
> const which is why I chose to make DeallocateElements() also const.
>
> For backwards compatibility reason, I think that AllocateElements()
> has to remain const. Also If I correctly understand the quote shown
> below, it is legal to call delete on a const pointer. My second fix is
> thus only a workaround for vs6 (as I mentioned in the comment):
> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkImportImageContainer.txx?root=Insight&r1=1.19&r2=1.20&sortby=date
>
> The reason why I made DeallocateElements() a const member taking a
> const pointer as argument is because I wanted to be consistent with
> the signature of AllocateElements(). I will be happy to investigate a
> change to make it non-const if you think that it is necessary.
>
> Best regards,
> Tom
>
>
> C++ Standard, Final Draft, Section 5.3.5, Paragraph 2
> If the operand has a class type, the operand is converted to a pointer
> type by calling the above-mentioned conversion function, and the
> converted operand is used in place of the original operand for the
> remainder of this section. In either alternative, if the value of the
> operand of delete is the null pointer the operation has no effect. In
> the first alternative (delete object), the value of the operand of
> delete shall be a pointer to a non-array object created by a
> new-expression, or a pointer to a subobject (1.7) representing a base
> class of such an object (10). If not, the behavior is undefined. In
> the second alternative (delete array), the value of the operand of
> delete shall be the pointer value which resulted from a previous array
> new-expression.68) If not, the behavior is undefined. [Note: this
> means that the syntax of the delete-expression must match the type of
> the object allocated by new, not the syntax of the new-expression. ]
> [Note: a pointer to a const type can be the operand of a
> delete-expression; it is not necessary to cast away the constness
> (5.2.11) of the pointer expression before it is used as the operand of
> the delete-expression. ]
>
>
> On Tue, Mar 31, 2009 at 09:21, Tom Vercauteren <tom.vercauteren at m4x.org> wrote:
>
>>Hi Bill and Luis,
>>
>>Thanks for the feedback. It might have taken some time to show up on
>>the dashboard but my second patch should have at least fixed the vs6
>>compilation issues:
>>http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkImportImageContainer.txx?root=Insight&r1=1.19&r2=1.20&sortby=date
>>
>>See the last dash13 build
>>http://www.cdash.org/CDash/index.php?project=Insight&date=2009-03-30#Continuous
>>
>>
>>On Tue, Mar 31, 2009 at 02:49, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>>
>>>Tom,
>>>
>>>BTW,
>>>The same goes for the AllocateElements() method, and
>>>the argument of DeallocateElements().
>>>
>>>It seems that none of them should be const.
>>>
>>>
>>> Luis
>>>
>>>
>>>--------------------------------------------------------------------------------------------------
>>>On Mon, Mar 30, 2009 at 8:44 PM, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>>>
>>>>Hi Tom,
>>>>
>>>>Why is that the method:
>>>>
>>>> DeallocateElements()
>>>>
>>>>needs to be const ?
>>>>
>>>>It is only called from non-const methods,
>>>>and the very nature of the method is that it alters
>>>>the state of the class.
>>>>
>>>>Have you find an scenario where the const
>>>>signature is needed ?
>>>>
>>>>
>>>> Thanks
>>>>
>>>>
>>>> Luis
>>>>
>>>>
>>>>----------------------------------------------------------------------------
>>>>On Mon, Mar 30, 2009 at 3:24 PM, Bill Lorensen <bill.lorensen at gmail.com> wrote:
>>>>
>>>>>Tom,
>>>>>
>>>>>Your changes are not compiling on VS6.
>>>>>http://www.cdash.org/CDash/viewBuildError.php?buildid=302665
>>>>>I don't have access to my VS6 system until next week. Perhaps Luis can helpout.
>>>>>
>>>>>Bill
>>>>>
>>>>>On Thu, Mar 19, 2009 at 9:14 AM, Tom Vercauteren
>>>>><tom.vercauteren at m4x.org> wrote:
>>>>>
>>>>>>Thanks Bill for the very interesting pointer!
>>>>>>
>>>>>>I think the patch really makes sense in that exact same scenario. If I
>>>>>>get it right, the factory basically loads a TestImportImageContainer
>>>>>>which is defined in itkFactoryTestLib.[h/cxx].
>>>>>>
>>>>>>TestImportImageContainer inherits from ImportImageContainer and
>>>>>>redefines AllocateElements. IMHO, if allocation is redefined, it makes
>>>>>>sense to also redefine deallocation.
>>>>>>
>>>>>>By enclosing all deallocation in one virtual function, this is what
>>>>>>the patch allows. Thus enabling for example the use of a
>>>>>>std::allocator instead of calling new and delete.
>>>>>>
>>>>>>Tom
>>>>>>
>>>>>>On Thu, Mar 19, 2009 at 16:51, Bill Lorensen <bill.lorensen at gmail.com> wrote:
>>>>>>
>>>>>>>Tom,
>>>>>>>
>>>>>>>Another possibility is to use the object factory mechanism to provide
>>>>>>>an override to ImportImageContainer. The test
>>>>>>>Testing/Code.Common/itkObjectFactoryTest2.cxx shows how
>>>>>>>ImportImageContainer can be overriden.
>>>>>>>
>>>>>>>Bill
>>>>>>>
>>>>>>>
>>>>>>>On Thu, Mar 19, 2009 at 7:51 AM, Tom Vercauteren
>>>>>>><tom.vercauteren at m4x.org> wrote:
>>>>>>>
>>>>>>>>Hi,
>>>>>>>>
>>>>>>>>At my company, we needed to have ITK images for which buffers are
>>>>>>>>ensured to be aligned (say on 8 bits). This can be done by overloading
>>>>>>>>itk::Image and itk::ImportImageContainer.
>>>>>>>>
>>>>>>>>However, it also requires a patch to ImportImageContainer for it to be
>>>>>>>>"overloadable". Basically, the attached patch only amounts to:
>>>>>>>>1) Putting all the memory deallocation in a single virtual function
>>>>>>>>2) Moving some member variables from private to protected
>>>>>>>>
>>>>>>>>There should be no backward compatibility issue with this. All unit
>>>>>>>>tests are still green on my computer.
>>>>>>>>
>>>>>>>>
>>>>>>>>By looking into ImportImageContainer, I also realized that Reserve is
>>>>>>>>wrong in the sense that it has a Resize semantics. This is similar to
>>>>>>>>the following VectorContainer and the MapContainer bug:
>>>>>>>>http://www.itk.org/Bug/view.php?id=2893
>>>>>>>>
>>>>>>>>Since it was decided not to fix bug 2893, I haven't fixed
>>>>>>>>ImportImageContainer::Reserve either but simply added some comments to
>>>>>>>>the file.
>>>>>>>>
>>>>>>>>Let me know if you see any potential problem with this patch.
>>>>>>>>
>>>>>>>>Thanks,
>>>>>>>>Tom
>>>>>>>>
>>>>>>>>_______________________________________________
>>>>>>>>Powered by www.kitware.com
>>>>>>>>
>>>>>>>>Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html
>>>>>>>>
>>>>>>>>Please keep messages on-topic and check the ITK FAQ at: http://www.itk.org/Wiki/ITK_FAQ
>>>>>>>>
>>>>>>>>Follow this link to subscribe/unsubscribe:
>>>>>>>>http://www.itk.org/mailman/listinfo/insight-developers
>>>>>>>>
>>>>>>>>
>>>>>>>
>
More information about the Insight-developers
mailing list