Fix unexpected type casting issues
Description
We encountered some issues when compiling for a different operating system where size_t is 32bit rather then 64bit. this causes the following simple example
void main() {
size_t x = 1;
int64_t y = -x;
std::cout << y << std::endl;
}
to unexpectedly output 4294967295 rather then -1.
Note: this PR is based on https://github.com/Ultimaker/CuraEngine/tree/NP-327_emscripten_communication; merge PR https://github.com/Ultimaker/CuraEngine/pull/2131 before this one.
Type of change
Prevent unexpected integer overflows by more explicitly casting.
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
How Has This Been Tested?
yes
Test Configuration:
- Operating System: MacOS 13.3
Checklist:
- [x] My code follows the style guidelines of this project as described in UltiMaker Meta
- [x] I have read the Contribution guide
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have uploaded any files required to test this change
NP-343
@jellespijker in the example I shared above the unexpected behaviour occurs when casting a smaller to a larger type due to the sign flipping. This is a different issue then you're trying solve with the stated example.
size_t x = 1; int64_t y = -x;
The example was intended as inspiration and a quick showcase. Maybe the example below is more suited and recognizable. https://godbolt.org/z/ocEs5aW71
#include <type_traits>
#include <concepts>
#include <cstdint>
template<typename T, typename U>
concept SafeConversion = requires {
// Check for arithmetic types
requires std::is_arithmetic_v<T> && std::is_arithmetic_v<U>;
// Ensure the size of T is at least the same as U to avoid truncation
requires sizeof(T) >= sizeof(U);
// Handle signed/unsigned conversions to prevent unintended behavior
requires (!std::is_integral_v<T> || !std::is_integral_v<U>) ||
(std::is_signed_v<T> == std::is_signed_v<U>) ||
(std::is_unsigned_v<T> && std::is_signed_v<U> && sizeof(T) > sizeof(U)); // Allow if unsigned T is larger
// Prevent direct comparisons between integral and floating-point types
requires (!std::is_integral_v<T> || !std::is_floating_point_v<U>) &&
(!std::is_floating_point_v<T> || !std::is_integral_v<U>);
};
int main() {
std::size_t x = 1;
std::int64_t y = -x;
static_assert(SafeConversion<decltype(x), decltype(y)>);
return y;
}
Resulting in:
<source>:26:19: error: static assertion failed
26 | static_assert(SafeConversion<decltype(x), decltype(y)>);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<source>:26:19: note: constraints not satisfied
<source>:6:9: required by the constraints of 'template<class T, class U> concept SafeConversion'
<source>:6:26: in requirements [with T = long unsigned int; U = long int]
<source>:14:14: note: nested requirement '((((! is_integral_v<T>) || (! is_integral_v<U>)) || (is_signed_v<T> == is_signed_v<U>)) || ((is_unsigned_v<T> && is_signed_v<U>) && (sizeof (T) > sizeof (U))))' is not satisfied
14 | requires (!std::is_integral_v<T> || !std::is_integral_v<U>) ||
| ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
15 | (std::is_signed_v<T> == std::is_signed_v<U>) ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
16 | (std::is_unsigned_v<T> && std::is_signed_v<U> && sizeof(T) > sizeof(U)); // Allow if unsigned T is larger
@jellespijker You really have a concepts-based solution for every situation :stuck_out_tongue:
In the present situation, this would indeed generate a proper error instead of a warning, but it doesn't actually fix the issue. What we discussed briefly with @casperlamboo would be to have our own types for everything (which we already do in the engine, partially), and proper conversion operators between them, so that you can have a compact, readable and error/warning-free code.
However we agreed for now to just fix this issue locally, and possibly add a C&C ticket to discuss this.
I've been trying to compile this in WASM, and I believe I found one more place that needs to be cast:
Raft::LayerType Raft::getLayerType(LayerIndex layer_index)
{
const auto airgap = Raft::getFillerLayerCount();
const auto interface_layers = Raft::getInterfaceLayers();
const auto surface_layers = Raft::getSurfaceLayers();
if (layer_index < -airgap - surface_layers - interface_layers)
On my architecture this ends up saying layer_index < Really big number.