Unsinged raster dimensions, allocation in size_t. No segfaults, yay.

[fester]

This PR fixes segmentation fault mentioned in #32 and other potential allocation related problems.

[adrpar]

@fester is there a reason not to support uint64_t? (and was there in general an argument for not using fixed sized integer types?)

Further, I think this should be covered by a test case.

[fester]

Do you mean using uint64_t for raster dimensions? Let's put it this way: given the scope of this utility, I can't see any reason to assume rasters larger than 4.2 billion pixels in any of the dimensions.

As for the test case, it's a good idea. Given the overflow happening in a private method and results in an incorrect heap memory allocation, the only way to test it is to check for the actual amount of memory allocated. Do you have any idea how to implement this kind of check in a cross platform fashion using standard C++ runtime?

[adrpar]

I meant for a general reason not to use fixed sized integers all over the code :) I think the dimensions used in issue https://github.com/heremaps/tin-terrain/issues/32 are realistic and should be supported if that is just an issue of using uint64_t (maybe something for the future, not this PR).

Testing is simple. Allocate such a large array and it should not segfault (I understood it that way, that with the old code and using those dimensions, it did segfault). Then again, if the testing machine does not have that much memory, this is also an issue... Ok, forget the testing bit :D

In order to address the original issue, we could add a sanity test either in the alloc (and just fail the alloc if the user requests a raster size we don't support) or in the calling code. And that can then be tested :) What do you think?

[fester]

I have a feeling we are not on the same page here.

Raster class implementation uses 4-byte ints ints to store raster dimensions. In order to allocate enough memory to hold the raster data, Raster's alloc() method calls new T[m_width*m_height]. The operator new[] expects a number of elements to be allocated as an 8-byte size_t, but because m_width and m_height are 4-bytes, compiler multiplies two 4-byte values resulting in a quiet overflow and new[] gets the leftover of the muliplication. This PR manually lifts ints to size_t telling compiler to generate 8-byte multiplication which doesn't overflow, producing a correct new[] call and a correct alocation. Since the allocation is correct now, segfault is gone and tin-terrain can accept any raster whose dimensions are under 2^32-1 pixels. It's up to OS to verify whether the system is capable of allocating needed amount of memory.

[adrpar]

No we are on the same page here. The only comment I had (as did Vova) was, that we might need to throw an error, to accommodate for any limitations we are having in the code. Since I am new to the code base, I cannot assess this yet and thus asked you to add an exception, if limitations should be imposed.

I will merge this now and if you feel an exception should be added, let's have another PR.

[adrpar]

+1