[Insight-developers] Win64 "unsigned long" -> size_t changes
Luis Ibanez
luis.ibanez at kitware.com
Tue Nov 23 19:20:06 EST 2010
Hi Dave,
Please see comments below.
Thanks
Luis
---------------
On Tue, Nov 23, 2010 at 12:20 PM, David Cole <david.cole at kitware.com> wrote:
> Questions and observations from my notes while making this change:
> http://review.source.kitware.com/#change,427
>
> Please read through and make any comments or replies as you see fit
> within your areas of expertise. These were things that I had questions
> about (I am non-expert on several ITK classes), and remain open for
> future discussions.
>
> Thanks,
> David Cole
>
>
> General: (other usages of "unsigned long" and "long" that remain:)
> - what are EquivalencyTable and EquivalencyTable used for? is
> "unsigned long" ok for it? or does it need 64-bit capacity? (i.e. --
> are PointIdentifier or CellIdentifier entities used in the equivalency
> tables? if so, they need 64-bit capacity...)
>
The EquvalencyTable is used in the Watershed filter.
They represent labels in an image. I will vote for making
them size_t, or at least explicit int64_t since 32 bit integers
(the Windows "long" type) may be insufficient for labeling
large images.
However, that probably should be a separate
patch from the one that fixes the image size
problem.
> - update / modified times remain as "unsigned long"
>
If we have a video application that refreshes a pipeline
with three filters we will need 6 unique updated times.
Let's say that we have
30 frames / sec and 86400 seconds in a day
This application will rollover the modified time
on Windows in 276 days of continuous run.
It looks a bit borderline,... but planning for the next
ten years, it probably make sense to make the
modified time type be a int64_t.
However, that probably should be a separate
patch from the one that fixes the image size
problem.
- DataObject source index remains as "unsigned int"
>
Yes, this is a counter on the
number of input / outputs of a filter.
2^32 should do the trick
> - event observer "tags"
>
Yes, this uniquely identifies command observers
so that they can be removed later. It will be unlikely
for an application to need more than 2^32 observers.
- number of dimensions
>
yeap, 2^32 should do it :-)
> - consider adding globally known typedefs for DimensionType and
> MTimeType and ObserverTagType
>
>
Yes, this will be great.
we should have use such typedef from the beginning...
> Specific questions about files in "Code/Common":
>
> ITK\Code\Common\itkAzimuthElevationToCartesianTransform.h
> ITK\Code\Common\itkAzimuthElevationToCartesianTransform.txx
> ?: azimuth and elevation expressed as "long", should be
> itk::Index::IndexValueType?
>
>
Yes, they refer to nodes of a grid with polar geometry.
IndexValueType is what they should use.
> ITK\Code\Common\itkBlox*
> ?:
>
number of boundary points, number of core atoms, number of itmes,
> number of pixels, size: all "unsigned long"
>
>
These are features identified inside an image.
"number of boundary points, number of core atoms, number of itmes,"
can live with unsigned long
But:
* Number of pixels should be: SizeValueType
ITK\Code\Common\itkBoundingBox
> ?: BoundingBox::TPointIdentifier changed default to "size_t" instead of
> "unsigned long", should it be *ValueType instead?
>
>
* size_t is probably the right one,...
it looks like we need the equivalent of vtkTypeId,
that will be in general a counter of instances for
collections that can have many instances.
Maybe called IdentifierType
typedef size_t IdentifierType
it could go in
itkIntTypes.h
ITK\Code\Common\itkBSpline*
> ?: ok as is? unsigned long used as weights count/index
>
>
This one should be
ImageType::IndexValueType
The BSpline is essentially an image.
ITK\Code\Common\itkCellInterface.h
> ?: cell id "unsigned long" -- should be instead?
>
>
size_t but defined through: IdentifierType
> ITK\Code\Common\itkCentralDifferenceImageFunction.txx
> ?: why the cast? is it necessary? was it to suppress warning?
>
>
The cast in line 73 is unnecessary,
start[dim] and index[dim] are of the same type.
while the one in 74 is needed because
"size[dim]" is unsigned long and it needs
to be involved in a subtraction operation
and then compared to another long.
That said, now it should be converted to
static_cast< IndexValueType >( size[dim] )
ITK\Code\Common\itkConstNeighborhoodIterator.h
> ?: GetBound return value?
>
>
the current line 134:
long GetBound(unsigned int n) const
should become
IndexValueType long GetBound(unsigned int n) const
ITK\Code\Common\itkConstNeighborhoodIterator.txx
> ?: re-write loop with "long"s left in it?
>
>
Line 53
::GetPixel(const unsigned n, bool & IsInBounds) const
should become
::GetPixel(SizeValueType n, bool & IsInBounds) const
line 128
::ComputeInternalIndex(unsigned int n) const
should become
::ComputeInternalIndex(SizeValueType n) const
and lines 131-134 :
long D = (long)Dimension;
unsigned long r;
r = (unsigned long)n;
should become
IndexValueType D = Dimension;
SizeValueTyep r = n;
ITK\Code\Common\itkCorrespondingList.h
> ?: changed to list::size_type
>
>
Yes.
> ITK\Code\Common\itkDefaultDynamicMeshTraits.h
> ?: changed to "size_t" from "unsigned long" but comment says it should be
> the "index type to the PointsContainer"
>
>
Yeap, this consistency is guarranted in line 140:
typedef MapContainer< PointIdentifier, PointType > PointsContainer;
So, just declare PointsContainer to be of type "size_t", in line 84.
but probably both of them using the generic "IdentifierType"
that we suggest to add to itkIntTypes.h as
typedef size_t IdentifierType
a less conflicting name could be
typedef size_t itkTypeId
(but we don't quite use that style in ITK...)
The same applies to lines 88 and 93
typedef unsigned long PointIdentifier;
typedef unsigned long CellIdentifier;
typedef unsigned long CellFeatureIdentifier;
ITK\Code\Common\itkDefaultImageTraits.h
> ?: first template parameter to ValarrayImageContainer changed to "size_t"
> instead of "unsigned long", should it be *ValueType instead?
>
This is a very important one !
It must be SizeValueType.
> ?: email to Luis and Brad :: REMOVE THIS FILE -- completely unreferenced
>
>
Yeap, nobody seem to use it.
I just moved it to the "itk-deprecated" module in the
Manifest.txt file for the modularization.
That will take care of it. :-)
> ITK\Code\Common\itkDefaultStaticMeshTraits.h
> ?: changed to "size_t" from "unsigned long" but comment says it should be
> the "index type to the PointsContainer"
>
>
Yeap, consistency is ensured in line 100:
typedef VectorContainer< PointIdentifier, PointType > PointsContainer;
We should make all the Identifier types (lines 80, 84, 89) to
be "IdentifierType" defined as "size_t" in itkIntTypes.h
> ITK\Code\Common\itkDirectory.cxx
> ok: "unsigned long" to match underlying implementation signature
>
>
Yeap,
It comes from kwsys.
> ITK\Code\Common\itkFixedArray.h
> ?: do we therefore also need long long and unsigned long long versions
> of operator[]?
>
>
This smells to Visual Studio 6.0...
Lines 194-208
/** Allow the FixedArray to be indexed normally. No bounds checking is
done.
* The separate versions are a work-around for an integer conversion bug
in
* Visual C++. */
I'm wondering if that compiler bug is still
in VS 7.1....-> 10 that we are supporting now...
The methods with different signatures shouldn't be necessary...
ITK\Code\Common\itkImageConstIteratorWithIndex.h
> ?: why does line 106 use " - 1" to compute pastEnd? shouldn't it be " +
> 1"?
>
>
It is explained in itkImageBase.h in lines 264-274:
"This table if of size [VImageDimension+1], because its
* values are computed progressively as: {1, N1, N1*N2,
* N1*N2*N3,...,(N1*...*Nn)} Where the values {N1,...,Nn} are the
* elements of the BufferedRegion::Size array. The last element of
* the OffsetTable is equivalent to the BufferSize."
> ITK\Code\Common\itkImageRandomConstIteratorWithIndex.txx
> ?: "PositionType" used to be "unsigned long" ... but it seems more
> like an Index or Offset
> not a Size... should it be signed?
>
>
SetNumberOfSamples() and GetNumberOfSamples() should use
SizeValueType (instead of unsigned long).
Couldn't find a "PositionType"
there is m_Position, that is a pointer to the buffer,
and there are "position" and "residual" variables
that are currently declared as "unsigned long".
They should be OffsetValueType.
> ITK\Code\Common\itkImageRandomNonRepeatingConstIteratorWithIndex.h
> ?: should first template parameter in "PriorityImageType" typedef be
> size_t or a reference
> to another ITK-defined type?
>
>
All of the following "unsigned long":
40: unsigned long m_Priority;
41: unsigned long m_Index;
71: unsigned long m_Size;
73: RandomPermutation(unsigned long sz)
91: void SetPriority(unsigned long i, unsigned long priority)
113: unsigned long operator[](unsigned long i)
291: typedef itk::Image< unsigned long,
itkGetStaticConstMacro(ImageDimension) > PriorityImageType;
319: void SetNumberOfSamples(unsigned long number);
321: unsigned long GetNumberOfSamples(void) const;
333: unsigned long m_NumberOfSamplesRequested;
334: unsigned long m_NumberOfSamplesDone;
335: unsigned long m_NumberOfPixelsInRegion;
should become
SizeValueType
ITK\Code\Common\itkIterationReporter.h
> ?: do we want more than 2 billion "steps per update"?
>
>
Yes.
most filters increment the iteration reporter once per pixel,
and this class decimate the frequency to about 100 calls
to update. So an image of 100 Gb will run out of values.
(we have a couple of 1Tb images from microscopy)
The same goes for the ProgressReporter class
ITK\Code\Common\itkLinearInterpolateImageFunction.h
> ok: probably ok to be limited to 2 billion neighbors for
> interpolation purposes...
>
>
Yes, that should be enough :-)
ITK\Code\Common\itkMesh.h
> ?: still need to change this: size_t -> proper type (CellIdentifier?)...
>
>
yes, GetNumberOfCells() should return CellIdentifier type.
the same for
GetCellBoundaryFeatureNeighbors()
and
GetCellNeighbors()
ITK\Code\Common\itkNumericTraits.h
> ?: class NumericTraits< int > :public vcl_numeric_limits< int >
> ...
> typedef long AccumulateType;
> AccumulateType should be a larger-sized type, right? int and long are
> the same (and both 32-bits) on Win32 and Win64
>
>
Yeap, good catch.
This should be IdentifierType.
typedef IdentifierType AccumulateType
ITK\Code\Common\itkNeighborhoodAlgorithm.txx
>
long overlapLow, overlapHigh;
overlapLow = static_cast< long >( ( rStart[i] - radius[i] ) - bStart[i]
);
overlapHigh = static_cast< long >( ( bStart[i] + bSize[i] ) - (
rStart[i] + rSize[i] + radius[i] ) );
should use "IndexValueType" instead of "long"
> ?: local "long" variables in ImageBoundaryFacesCalculator< TImage
> >::operator()
> should be "long long"?
>
>
nope, just IndexValueType should do it.
They are computing indexes locations.
> ITK\Code\Common\itkPointSet.h
> ?: "typedef long RegionType;" should be larger than "long"?
>
>
Yes.
In a worst case scenario partition there will be
as many regions as points.
This should be "IdentifierType" (e.g. like vtkTypeId)
> ITK\Code\Common\itkQuadEdgeCellTraitsInfo.h
> ?: remaining "unsigned long" -- should be size_t, TPointIdentifier, other?
>
typename TPointIdentifier = unsigned long,
typename TCellIdentifier = unsigned long,
typename TPointsContainer = MapContainer< unsigned long, TPoint >,
typename TQE = GeometricalQuadEdge< unsigned long, unsigned long,
bool, bool, true > >
should be using "IdentifierType" (e.g. like vtkTypeId)
instead of unsigned long.
> ITK\Code\Common\itkQuadEdgeMeshPoint.h
> ?: same : "unsigned long" here?
>
>
template< class TCoordRep, unsigned int VPointDimension, typename TQuadEdge
=
GeometricalQuadEdge< unsigned long, unsigned long, bool, bool,
true > >
should be using "IdentifierType" (e.g. like vtkTypeId)
instead of unsigned long.
> ITK\Code\Common\VNLIterativeSparseSolverTraits.h
> ok: leave alone... do we need more than 2 billion iterations here?
>
Unlikely,
but there is no reason for not having a larger type.
The allocation extra cost is insignificant.
Not good reason to change it either...
so leaving it alone is fine.
---
Luis
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20101123/038be996/attachment.htm>
More information about the Insight-developers
mailing list