Do not cast pointers to arrays of differently sized integers. #485
Conversation
mmuetzel
requested review from
balos1,
gardner48,
cswoodward and
drreynolds
as code owners
There was a problem hiding this comment.
As long as we can reliably count on compilers to catch the size discrepancy and halt with an error, I think this is great. That said, to head off user GitHub issues complaining that SUNDIALS won't compile, it would be ideal to combine this change with a corresponding check for type compatibility at the CMake level.
|
@mmuetzel Looks like clang-format is failing. You can go to the failed job, download the "diff" artifact and apply the patch with git to fix the styling without installing clang-format. |
Casting between `int64_t *` and `int32_t *` does *not* maintain the values in the array. Instead, it tells the compiler to interpret the memory at that pointer as an array of a different type (i.e., two `int32_t` elements "become" one `int64_t` element). That can lead to all kinds of errors and is very not what was intended. Remove the integer pointer casts to allow the compiler to emit an error on compile-time instead of potentially causing, e.g., an array overflow on runtime if `sunindextype` has a different size from `KLU_INDEXTYPE`. Signed-off-by: Markus Mützel <markus.muetzel@gmx.de>
I don't know if all compilers will reliably error if the pointer types don't match. They might only emit a warning or don't have a diagnostic for that at all. But with the cast, even compilers that have that diagnostic wouldn't be able to detect that there is an issue. So, I think this is a step in the right direction. Do you prefer to add the check during configuration in this PR? Or would it be ok if I opened a separate PR for that?
Thanks for the hint. I downloaded that diff and merged it into the commit for this PR. |
I agree.
I'm okay with either. Perhaps another developer has a preference? |
|
@mmuetzel Please add the CMake check to this PR. |
…type If `sunindextype` is a 64-bit integer, `SuiteSparse_long` must also be a 64-bit integer or KLU cannot be used. Add a CMake check to fail during configuration if the sizes don't match. Signed-off-by: Markus Mützel <markus.muetzel@gmx.de>
|
The new commit adds a configure check for the size of |
Casting between `int64_t *` and `int32_t *` does *not* maintain the values in the array. Instead, it tells the compiler to interpret the memory at that pointer as an array of a different type (i.e., two `int32_t` elements "become" one `int64_t` element). That can lead to all kinds of errors and is likely not what was intended. Remove the pointer casts to allow the compiler to emit an error on compile-time instead of potentially causing, e.g., an array overflow on runtime if `sunindextype` has a different size from `KLU_INDEXTYPE`. --------- Signed-off-by: Markus Mützel <markus.muetzel@gmx.de> Co-authored-by: Cody Balos <balos1@llnl.gov>
Casting between
int64_t *andint32_t *does not maintain the values in the array. Instead, it tells the compiler to interpret the memory at that pointer as an array of a different type (i.e., twoint32_telements "become" oneint64_telement). That can lead to all kinds of errors and is likely not what was intended.Remove the pointer casts to allow the compiler to emit an error on compile-time instead of potentially causing, e.g., an array overflow on runtime if
sunindextypehas a different size fromKLU_INDEXTYPE.