WIP: Tweak FixedArray and derived constructors

[hjmjohnson]

Niels suggested the change from ValueType[Dimension] (c-type ValueType*)
to ValueType(&)[Dimension].
It is better now doubt, but it generates many breaking changes. That I
do think it could be worth to fix. The main one that is too big to
tackle right now is with all the interface of Cells.

This had to be inherited from VTK, but the interface in ITK could
enjoy a better interface than current raw pointers.
VTK does use c-arrays instead of pointers.
Compare VTK:
https://github.com/Kitware/VTK/blob/a7909e67d7df88dcf81984923296442c7ccaca5b/Common/DataModel/vtkCell.h#L197-L199
With ITK:

ITK/Modules/Core/Common/include/itkCellInterface.h

Lines 320 to 326 in 3a6158a

	virtual bool EvaluatePosition(CoordRepType *,
	PointsContainer *,
	CoordRepType *,
	CoordRepType[],
	double *,
	InterpolationWeightType *)
	{ return bool(); }

ENH: Provide constructor from C-array to FixedArray

Also derived classes of FixedArray, inherit constructors from base.
using Superclass::Superclass
This allow to remove some repetition and homogenize the interface.

The only possible breaking change is marking as explicit two FixedArray
converting constructors:

explicit FixedArray(const ValueType & );

template< typename TFixedArrayValueType >
  explicit FixedArray(const FixedArray< TFixedArrayValueType, VLength > & r)

The template conversion is needed to avoid unexpected conversion in
derived classes. Also marking as explicit is in general a good practice.
Users might use static_cast instead of relying in the non-existant
implicit conversion.

ITK_LEGACY_FUTURE_REMOVE was used to switch between the explicit or
implicit constructor in itkVector. The switch is removed, that Vector behavior
is now handled from its base class (FixedArray).

COMP: Fix uninitialized FixedArray and const correctness

This is a false positive, the variables are not used uninitialized, but
we can solve it using the conversion constructor of FixedArray:

FixedArray(Value r[VLenght])

Also add missing const in SetNormal in LineSpatialObject

Fix warning:

Modules/Core/SpatialObjects/include/itkSpatialObjectPoint.hxx:
In member function ‘itk::MetaTubeConverter<NDimensions>::SpatialObjectPointer itk::MetaTubeConverter<NDimensions>::MetaObjectToSpatialObject(const MetaObjectType*) [with unsigned int NDimensions = 3]’:
Modules/Core/SpatialObjects/include/itkSpatialObjectPoint.hxx:172:3: warning: ‘point’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   m_X = newX;
   ^~~
Modules/Core/SpatialObjects/include/itkSpatialObjectPoint.hxx:
In member function ‘itk::MetaVesselTubeConverter<NDimensions>::SpatialObjectPointer itk::MetaVesselTubeConverter<NDimensions>::MetaObjectToSpatialObject(const MetaObjectType*) [with unsigned int NDimensions = 3]’:
/home/user/Software/ITK/ITK-src/Modules/Core/SpatialObjects/include/itkSpatialObjectPoint.hxx:172:3: warning: ‘point’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   m_X = newX;

[hjmjohnson]

http://review.source.kitware.com/#/c/23836/

Pablo He...z-Cerdan
Uploaded patch set 1.Oct 27 9:34 AM

Pablo He...z-Cerdan
Uploaded patch set 2: Commit message was updated.Oct 27 9:36 AM

Pablo He...z-Cerdan
Uploaded patch set 3.Oct 27 9:39 AM

Dzenan Zukic
Patch Set 3: Code-Review+1Oct 27 9:45 AM

Niels Dekker
Oct 27 10:33 AM
↩
Patch Set 3:

(2 comments)

Cool! Looks like this fix allows you to declare the variables 'const', right? I would like that even better :-)

Modules/Core/SpatialObjects/include/itkMetaLineConverter.hxx
Line 78:
const PointType...?

Line 83:
const NormalType...?

Pablo He...z-Cerdan
Uploaded patch set 4.Oct 27 11:01 AM

Pablo He...z-Cerdan
Patch Set 4: If you like it, you have it! This const correction showed a missing const qualifier in SetNormal argument. Added it.Oct 27 11:03 AM

Dzenan Zukic
Patch Set 4: Code-Review+1Oct 27 11:11 AM

Kitware Build Robot
Patch Set 1: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23836-1&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 27 11:21 AM

Kitware Build Robot
Patch Set 3: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23836-3&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 27 11:29 AM

Pablo He...z-Cerdan
Patch Set 4: Code-Review-1 Ok, it is using the other conversion constructor, will try to fix it.Oct 27 11:45 AM

Kitware Build Robot
Patch Set 4: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23836-4&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 27 2:05 PM

Pablo He...z-Cerdan
Uploaded patch set 5.Oct 27 8:09 PM

Kitware Build Robot
Patch Set 5: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23836-5&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 27 9:22 PM

Pablo He...z-Cerdan
Uploaded patch set 6.Oct 28 12:06 PM

Kitware Build Robot
Patch Set 6: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23836-6&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 28 12:38 PM

Pablo He...z-Cerdan
Patch Set 6: All these errors, triggered by adding explicit to FixedArray conversion constructors are solvable with static_cast. The question is, what do we want to do? I find the implicit conversions to look beautiful and easy to use, but they hide too much. For example: result_vector = vector - point, it seems like a cheaper operation than it really is. There is an implicit conversion of point, from Point to Vector with the constructor Vector(const &point), and then the Vector::operator-(const Vector&) This may be unavoidable, but we can also implement a Vector::operator-(const Point &) to avoid the extra call to the constructor. Also the example of ArrayType arr = 2.0; I think an explicit ArrayType(2.0) makes it cleaner, or static_cast(2.0) will do as well. Anyway, I think explicit should be the default behaviour in FixedArray, and maybe let the more mathematical Vector, Point with the old non-explicit conversions. I will add operator-, operator+ for FixedArray of different ValueType. To save that extra constructor. But we can start the discussion.Oct 28 1:30 PM

Pablo He...z-Cerdan
Patch Set 6: Code-Review-1Oct 28 1:30 PM

Niels Dekker
Oct 28 1:58 PM
↩
Patch Set 6:

(1 comment)

@pablo, another aspect below here, please consider:

Modules/Core/Common/include/itkFixedArray.h
Line 145:
Please note that for this declaration, 'FixedArray(const TScalarType r[VLength])', the compiler will not check the length of the argument. A user may do 'itk::FixedArray<int, 3> fixedArray = cArray', even when her 'cArray' does not have 3 elements. If you want to prefer such an error at compile-time, declare the constructor as follows:

template
FixedArray(const TScalarType (&r)[VLength])

Niels Dekker
Patch Set 6: Typo: "prefer such an error..." should be "prevent such an error..."Oct 28 2:01 PM

Pablo Hernandez-Cerdan
Oct 28 9:14 PM
↩
Patch Set 6:

(1 comment)

Modules/Core/Common/include/itkFixedArray.h
Line 145:
It sounds good, but breaking changes, or at least propagating changes are needed. The current c syntax TScalarType[VLength] is really a TScalarType*, if we replace it by c++ syntax (with the reference), the function won't accept raw pointers anymore.

With this change, starting as early as in SetOrigin, SetSpacing in itkImageBase, propagates a bit. I think is for better, but again, good to discuss.

Thanks Niels, next patch introduces your proposed change.

Pablo He...z-Cerdan
Uploaded patch set 7.Oct 28 10:09 PM

Pablo He...z-Cerdan
Patch Set 7: This is becoming too big of a (WIP) change... I might drop Niels suggestion into another patch.Oct 28 10:11 PM

Kitware Build Robot
Patch Set 7: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23836-7&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 28 10:38 PM

Niels Dekker
Patch Set 1: @pablo It's fine by me you put my suggestion (or something similar) into another patch. I also think that it is becoming too big for a single commit. Your original itkMetaLineConverter.hxx fix from http://review.source.kitware.com/#/c/23836/1 becomes somewhat obscured by all the other changes.Oct 29 6:26 AM

Dzenan Zukic
Oct 29 8:22 AM
↩
Patch Set 7: Code-Review+1

(4 comments)

Commit Message
Line 11:
no

Modules/Core/Common/include/itkFixedArray.h
Line 191:
to

Modules/IO/SpatialObjects/test/itkReadWriteSpatialObjectTest.cxx
Line 937:
Position

Modules/Registration/Metricsv4/test/itkMeanSquaresImageToImageMetricv4VectorRegistrationTest.cxx
Line 270:
break line here

Matt McCormick
Oct 29 5:34 PM
↩
Patch Set 7:

Cool stuff!

Breaking out into separate patches should help.

What changes can we make without breaking existing code?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[dzenanz]

Who was spearheading this patch?

[dzenanz]

/azp run all

[azure-pipelines]

No pipelines are associated with this pull request.

[hjmjohnson]

It is assigned to @phcerdan so I assume that he is the lead. I don't think it is a priority.

[phcerdan]

I am not going to tackle this in a foreseeable future. Closing as the stale bot did.