implicit signed/unsigned conversion and truncations

[pizzard]

The code is littered with problematic conversions from unsigned to signed integers and truncations, because size_t has 64 bit length and int hasn't.

Some of them are issues due to the fact that the Template parameter IndexType is not used where it ought to be, this is relative easy to fix (await pull request).

But a big problem here is that the Dimension is of type int instead of type IndexType, so if you set IndexType to size_t and have a int dimension you will get buried in hundreds of warnings.
Solutiuon would be to change the Template argument order and make the dim paramter of type IndexSize, but this is a serious Interface change and needs disuccion.

Any ideas how to fix this?

[jlblancoc]

Thanks for reporting!
I think there shouldn't be any problem with backwards compatibility doing it like this:

• Switching the template parameter default type: int -> size_t (as it should have been from the beginning, I agree). The API shouldn't be broken, and we don't have ABI to worry about...
• Wrapping its special value "-1" everywhere with static_cast<size_t>(-1)

I can't work right now on trying it, but if you can and are satisfied with the result, please feel free to open another PR!

[pizzard]

Acutally what you might want to do is have the dimension type of the same type as the IndexType points to.
Therefore the order of the template paramters would have to be switched.
I think a there is a legitimate way to avoid this breaking API change is you take the dim paramter as size_t but inside the class typedef a DimensionType which is the same an IndexType und explicitly cast the template parameter value where you need it to DimensionType.

Your last template paramter intends to leave that choice of which type holds your indices to the user, then you should for the dimension also really leave it to the user.
Because if the user uses a 64bit datatype there the user may rely of having no truncation of that value, then we should fulfill that.

[vladimir-ch]

It is a mistake to use unsigned integers for indices (and therefore for sizes because of mixing signed/unsigned types), see
http://channel9.msdn.com/Events/GoingNative/2013/Interactive-Panel-Ask-Us-Anything
at 42:38 for a trustworthy explanation.

nanoflann provides adaptors for Eigen classes, so for example the default index type of Eigen for dense matrices is std::ptrdiff_t.

[jlblancoc]

@vladimir-ch Thanks for the interesting discussion from the C++ gurus!
It's great to follow STL decision of making all sizes size_t = unsigned, for these guys now to come recognize it was a mistake! Their reasons make a point.

[jlblancoc]

Fixed after 7011ba3.