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.11k stars 256 forks source link

const qualifiers in pubic API #845

Open martinjrobins opened 1 week ago

martinjrobins commented 1 week ago

Is your feature request related to a problem? Please describe.

I'm writing a rust sys crate that uses bindgen to automatically generate rust bindings for suitesparse. At the moment when wrapping a function like klu_analyze in the KLU solver, raw pointers are wrapped as mutable pointers *mut T, e.g: the C function

KLU_symbolic *KLU_analyze       /* returns NULL if error, or a valid
                                   KLU_symbolic object if successful */
(
    /* inputs, not modified */
    Int n,              /* A is n-by-n */
    Int Ap [ ],         /* size n+1, column pointers */
    Int Ai [ ],         /* size nz, row indices */
    /* -------------------- */
    KLU_common *Common
)

becomes

extern "C" {
    pub fn klu_analyze(
        n: i32,
        Ap: *mut i32,
        Ai: *mut i32,
        Common: *mut klu_common_,
    ) -> *mut klu_symbolic_;
}

This is an issue because the arguments Ap and Ai are incorrectly marked as mutable, causing problems downstream for users who are then required to have mutable handles to Ap and Ai. This can be corrected by marking the arguments Ap and Ai in the C signature as const, for example:

KLU_symbolic *KLU_analyze       /* returns NULL if error, or a valid
                                   KLU_symbolic object if successful */
(
    /* inputs, not modified */
    Int n,              /* A is n-by-n */
    Int Ap [const],         /* size n+1, column pointers */
    Int Ai [const],         /* size nz, row indices */
    /* -------------------- */
    KLU_common *Common
)

which would then generate the correct rust function signature with Ap and Ai marked as `const.

Are there any plans to explicitly use const qualifiers in suitesparse rather than relying on comments like this? Would you be happy if I put together a PR adding this to the functions that I need?

Describe the solution you'd like

Use explicit const qualifiers in public facing functions.

Describe alternatives you've considered

The alternative is for the bindings for suitesparse be manually implemented, rather than relying on automated tools.

mmuetzel commented 1 week ago

I agree with your point. It would probably be best if function arguments that are pointers (or arrays) to variables that aren't allowed to be changed by the function were marked as const.

However, are you sure that Int Ap[const] is valid C syntax? Shouldn't it be const Int Ap[] instead?

martinjrobins commented 1 week ago

I believe you can do Int Ap[const] since C99 (https://en.cppreference.com/w/c/language/const#:~:text=In%20a%20function%20declaration%2C%20the,the%20array%20type%20is%20transformed.), but I'm not sure as this is the first time I've come across this syntax. Presumably const Int Ap[] is equivilent?

UPDATE: actually, after looking at https://en.cppreference.com/w/c/language/array I think const Int Ap[] is a pointer to an array of const Int values, whereas Int Ap[const] is a const pointer to an array of Int values, so I think you are correct it should be const Int Ap[], or perhaps even const Int Ap[const].

DrTimothyAldenDavis commented 6 days ago

This will take me quite a while to resolve, since I would need to revise all APIs to essentially all my packages. It might also require an SO change to all my packages as well. But I'll consider it for the future.