[Insight-developers] Small modification to ImportImageContainer

Tue Mar 31 03:36:27 EDT 2009

(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
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>