search

DrTimothyAldenDavis / SuiteSparse

The official SuiteSparse library: a suite of sparse matrix algorithms authored or co-authored by Tim Davis, Texas A&M University.
https://people.engr.tamu.edu/davis/suitesparse.html
Other
1.17k stars 262 forks source link

[ParU] Replace C-style casts by static_cast's #635

Closed gruenich closed 10 months ago

gruenich commented 10 months ago

I tried to use reinterpret_cast, too. Cppcheck warns that these could be potentially not portable (casting between long and double) and GCC does not accept these casts within constexpr functions. Thus, I retract my changes to introduce constexpr and reinterpret_cast. Probably C++20's provides a portable solution by std::bit_cast.

mmuetzel commented 10 months ago

This looks good to me in general. But using malloc, calloc, ... and free isn't really C++-style anyway. So, I don't know why you'd use C++-style casts here. (It doesn't hurt though afaict.) Would it be possible to use new [] and delete [] operators instead?

DrTimothyAldenDavis commented 10 months ago

All the packages in SuiteSparse have to be able to use the same underlying memory management routines. That's why I have SuiteSparse_malloc and friends. If I use new and delete then they'd have to be placement new and such, with malloc underneath.

gruenich commented 10 months ago

So, I don't know why you'd use C++-style casts here. (It doesn't hurt though afaict.)

You are right. I have an agenda:

  1. Fix compiler warnings for GCC and Clang with -Wall -Wpedantic. This is mostly done, I am satisfied.
  2. Fix static analyzer warnings which can be fixed. Some progress for Cppcheck and Clazy.
  3. Use clang-tidy to use more C++ code instead of C code. This is rather limited. Nevertheless, locally some C arrays might be replaceable by std::array or similar.

I plan to do this in smaller steps, as Tim's unusual constraints (no judging) require some changes to be thrown away.

DrTimothyAldenDavis commented 10 months ago

The constraints I have on malloc/free are due to the use of SuiteSparse in many applications, and I can't change it without breaking them. Nearly all large applications impose a uniform control over their memory allocations, and I have to follow that or else the code cannot be used. It's not my constraint.

gruenich commented 10 months ago
  1. Sorry Tim for teasing you. I totally understand the constraints and that it is not your choice. :smile:
  2. I think for some time SuiteSparse should stick to the current memory management. At some point, all libraries should switch to C++ or have at least a CMake option to turn on a new C++-based model.
  3. Do you mind merging this pull request? I agree with Markus that more would be possible, but it remains an improvement.
DrTimothyAldenDavis commented 10 months ago

ParU needs lots of work, for these things and more. I need to think through the entire API in particular.

I'd like to do that after I release a stable 7.4.0, however. So I would like to wait and merge it in a couple days.

I can merge it just after I release a stable 7.4.0.

DrTimothyAldenDavis commented 10 months ago

Oh ... I just merged it into the wrong branch. I missed that. Please apply all PR's to the dev2 branch, not dev.

DrTimothyAldenDavis commented 10 months ago

Fixed here: https://github.com/DrTimothyAldenDavis/SuiteSparse/commit/07bb52afb5526f5fd7a58eb193e51412734e1506