Use const pointers in signature of dgstrs?

[maxaehle]

The current signature of dgstrs is

void
dgstrs (trans_t trans, SuperMatrix *L, SuperMatrix *U,
        int *perm_c, int *perm_r, SuperMatrix *B,
        SuperLUStat_t *stat, int *info)

Note that the arguments perm_c and perm_r, which specify column and row permutations, are used in a read-only fashion. It would thus be possible to declare them as int const* instead of int*. Such an API change would clarify the semantics to the users, and allow them to pass the same array two times (e.g. {0,1,2,...} to indicate that there are no row and column permutations). Such an API change should not break existing code as

A pointer to an unqualified type may be implicitly converted to the pointer to qualified version of that type (in other words, const, volatile, and restrict qualifiers can be added).

(cited from here).

The analogous observations holds for the variants of dgstrs concerned with other data types. I have not checked other SuperLU functions regarding whether pointers in the function signatures can be made const pointers.

[gruenich]

I try to tackle this issue in a systematic manner. Cppcheck warns these cases with constParameterPointer. I started to fix them, see #151. It is not completed, I ensured dgstrs is covered. Can you give it a try? I agree that adding const makes the interface more telling what the user can expect. You are not expecting any performance improvement, right?

[maxaehle]

Thanks for looking into this! Yes, the main argument for qualifying pointer arguments as const is to provide more information to users, and I don't expect performance improvements in the SuperLU code. However, user code calling SuperLU may be written in a faster way if it can rely on constness. E.g. in https://github.com/scipy/scipy/pull/17924#discussion_r1615813192, we want to pass the same arrays for perm_c and perm_r, but we can only pass the same pointer to a single array twice (as opposed to pointers of two separately allocated and initialized arrays) because we are sure that the function does not modify the arrays. Right now we are sure because we checked the implementation, with const we can be sure just from looking at the signature.

[gruenich]

I started with two dozed places in #151. There are more then 450 pointers left, that should be passed as const pointers. I don't think the effort and such a massive change is worth the benefit.

Max and @xiaoyeli , what do you think? Should we close this issue and #151 as won't fix? Or is an improvement worth the investment/changes?

[xiaoyeli]

This code started more than 20 years ago, when 'const' in C is not a big thing. I won't have time to go back fix them all. But I am happy to follow this practice for future new code development.

[gruenich]

Sherry, let me rephrase my question: Do you think it is worth to invest (mainly my) time into changing almost 500 places and risking breaking things for users (e.g.,by them having forward declaration that are changed, it remains API changes). Or would you prefer not to merge this approach to close this issue? I am fine either way.

But I am happy to follow this practice for future new code development.

Great to hear that, thank you!

[xiaoyeli]

I merged your PR#151. I think we don't need to do this extensively now, just to fix things on the way.

[gruenich]

@xiaoyeli This can be closed.