[Insight-users] Image offset is giving bad pointer for large datasets (7Gb)

[Luis Ibanez]

Mike,

Yeap, all those "implicit" uses of "long" are bugs
and must be fixed.

All this now falls under the umbrella of Bug 9260.

Unfortunately there is no shortcut on how to fix these
issues, we have to go one by one....

As you pointed out, right now we are focusing on fixing
Index, Offset and Size, but... any other uses of "long"
are suspect...

In retrospective, this is the result of being sloppy in our
practices of Generic Programming, and allowing our
knowledge of the Index component being "long" to
percolate in the downstream code.

We should have been using Traits from Index, Size
and Offset all this time.



      Luis


----------------------------------------------------------------------
On Mon, Jul 13, 2009 at 12:04 PM, Michael Jackson <
mike.jackson at bluequartz.net> wrote:

>
> ---
> Mike Jackson                 www.bluequartz.net
>
>
>
> On Jul 13, 2009, at 11:30 AM, Bradley Lowekamp wrote:
>
>  Luis,
>>
>>        This looks like an absolutely massive effort you are attempting
>> here. I have grepped through the iterator classes are there alot of
>> incorrect usages of long:
>>
>> itkImageRegionReverseConstIterator.h:    m_SpanEndOffset =
>> this->m_BeginOffset - static_cast<long>(this->m_Region.GetSize()[0]);
>>
>
> So that is a "bug" and should be corrected. This effort will help get rid
> of those.. I hope. Turning on -Wall may help track those down a bit easier
> but with the size of the code base of ITK there is going to be a large
> effort to track those down.
>
>
>> And then there are complex relations like this:
>>
>> itkSize.h:  typedef   unsigned long     SizeValueType;
>> itkImageRegion.h:  typedef Size< itkGetStaticConstMacro( ImageDimension )
>> >  SizeType;
>> itkImageRegion.h:  typedef typename SizeType::SizeValueType
>>    SizeValueType;
>> itkImageRegion.h:  SizeValueType GetNumberOfPixels() const;
>>
>>
>> I am not really sure how all these issues can be tracked down. From the
>> user list:
>>
>>
>> On Jul 10, 2009, at 12:46 PM, Michael Jackson wrote:
>>
>>
>>> IMNSHO, ITK (and VTK by extension) should absolutely BAN the use of
>>> "long" in their projects. Period. Too many avoidable bugs come up from
>>> its use. If you want a 32 bit integer use int or the standard ansi int
>>> type, if you want a 64 bit integer then use "long long int" or the
>>> standard ansi 64 bit integer type.
>>>  "long int" is just a mess waiting to happen. Maybe a rule should be
>>> put into the KWStyle project to look for and flag the use of 'long'?
>>>
>>
>>
>> Do we need to go as far as banning the use of long and unsigned long?
>>
>>
>> Good Luck Luis,
>> Brad
>>
>
> I stated that without fully understanding what the SizeValueType really
> represented. For this case I think Luis has settled on using ptrdiff_t
> instead of "long". But I _still_ stand by my opinion that any use of "long"
> is bound to have really bad subtle bugs which are hard to find and fix.
>
>
> Mike Jackson
>
>
>
>> On Jul 10, 2009, at 6:33 PM, Luis Ibanez wrote:
>>
>>  On recent realization:
>>>
>>>             "size_t"    is unsigned       :-/
>>>
>>> and we need the OffsetValueType to be signed,
>>> since we use it to compute differences...
>>>
>>> It seems that what we need is the type
>>>
>>>                        "ptrdiff_t"
>>>
>>> which is supposed to represent the differences
>>> between two pointers, and therefore should be
>>> capable of measuring distances between any
>>> two locations in memory.
>>>
>>> I'm now rerunning the Experimental with
>>> "ptrdiff_t" instead of "size_t".
>>>
>>>
>>>       Luis
>>>
>>>
>>> ------------------------------------------------------------------------------------------------------
>>> On Fri, Jul 10, 2009 at 12:30 PM, Luis Ibanez <luis.ibanez at kitware.com>
>>> wrote:
>>> Hi Kana,
>>>
>>> Thanks a lot for looking into this and sharing your findings.
>>>
>>> I just confirmed that in Windows 64bits, the "long" type is
>>> only 4 bytes.
>>>
>>> Here is the program I used:
>>>
>>> #include <iostream>
>>> #include "itkOffset.h"
>>> #include "itkNumericTraits.h"
>>>
>>>
>>> int main()
>>> {
>>>  unsigned long tt;
>>>  std::cout << "size = " << sizeof(tt) << std::endl;
>>>  tt  =  -1;
>>>  std::cout << "tt = " << tt << std::endl;
>>>
>>>  typedef itk::Offset<3>   OffsetType;
>>>  typedef OffsetType::OffsetValueType   OffsetValueType;
>>>
>>>  OffsetValueType  offsetValue;
>>>
>>>  std::cout << "sizeof(offsetValue) = " << sizeof( offsetValue ) <<
>>> std::endl;
>>>
>>>  offsetValue = itk::NumericTraits< OffsetValueType >::max();
>>>
>>>  std::cout << "OffsetValueType max() = " << offsetValue << std::endl;
>>>
>>>  return EXIT_SUCCESS;
>>> }
>>>
>>>
>>> with this CMakeLists.txt file
>>>
>>>
>>> CMAKE_MINIMUM_REQUIRED(VERSION 2.4)
>>> IF(COMMAND CMAKE_POLICY)
>>>  CMAKE_POLICY(SET CMP0003 NEW)
>>> ENDIF(COMMAND CMAKE_POLICY)
>>>
>>>
>>> PROJECT(64bitsTest)
>>>
>>> FIND_PACKAGE(ITK REQUIRED)
>>> INCLUDE(${ITK_USE_FILE})
>>>
>>> ADD_EXECUTABLE(typesTest typesTest.cxx )
>>>
>>> TARGET_LINK_LIBRARIES(typesTest ITKCommon)
>>>
>>>
>>> <ATT00001.txt>
>>>
>>
>> ========================================================
>> Bradley Lowekamp
>> Lockheed Martin Contractor for
>> Office of High Performance Computing and Communications
>> National Library of Medicine
>> blowekamp at mail.nih.gov
>>
>
> _____________________________________
> 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-users
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/pipermail/insight-users/attachments/20090713/3c0eb297/attachment-0001.htm>