Add -Wshadow -Wpointer-arith -Wcast-qual -Wsign-compare

[dinosaure]

This patch mainly try to lint our codebase with few warnings. It introduces also the usage of _Generic (C11) to be able to keep const qualifier between arguments and returned values (à la Rust).

[dinosaure]

I think it's ready to merge. The change is not really deep, just add few const and the usage of _Generic (C11) to be able to keep const between arguments and returned values. We are 2026, I think it's ok to move on. Probably @MisterDA will be interesting also by this PR.

[hannesm]

I struggle with the gcc specific enabling and disabling of specific warnings, can we instead fix the code and have gcc not warn?

[dinosaure]

I struggle with the gcc specific enabling and disabling of specific warnings, can we instead fix the code and have gcc not warn?

Unfortunately, these pragma is related to a not safe but common pattern in C about const.

• here the comment above explain why we should be able to "unconst" safely
• here, the name of the function is explicitly unconst
• and about mft_get_by_{name,index}, it's the same story because on one side, we would like to write data and, on the other side, we should only have a read access. This is why we have the _Generic which ensures that if we gives a const, we should get a const but if we give a value (which can be written), we also return a value which can be writtern

May be my experience with C is not enough to find a "good" example, but as far as I can say, the pattern to "unconst" unsafely (according to Wcast-qual) returned values seems common in C.

[dinosaure]

Probably @haesbaert or @Kensan have more knowledge (and a good opinion) about const in C 👍

[dinosaure]

I finally found something which seems good enough (at least, for my perspective). I deleted #pragma and use _Generic to provide a const function of mft_get_by_{name,index} & mft_get_builtin_mft1. I followed the same style with the _unconst function and use the _Generic to dispatch correctly the caller to the right implementation (and, I suspect that the compiler is able to optimize at some points).

A fix is needed for the virtio backend where we would like to set virtio_manifest to where we have our manifest but as a const value. virtio has an empty entry and we don't verify if this one was correctly attached or not.

The only remaining pragma is also about virtio when we would like to write a page into a block device but I think this one is well documented to say: it's safe to ignore -Wcast-qual in that case.

[MisterDA]

Your MR reminds me of N2973 Qualifier-preserving standard library functions. You may find something that helps in the "Implementation" subsection. I've also found https://inbox.sourceware.org/libc-alpha/mvm5x8wpsmi.fsf@suse.de/T/.
I also suggest the -Wwrite-strings C compiler flag.

[dinosaure]

Thanks @MisterDA, I was able to remove few lines and propose something more clean than before (without repetition). I also applied -Wwrite-strings. I think, regardless documentation, this PR is good for me.

[dinosaure]

A gently ping to @MisterDA to have a formal review (and if @Kensan or @haesbaert would like to participate).

[MisterDA]

Just a note, in my experience -Wcast-qual doesn't play well with the OCaml or POSIX API. Sometimes you need to cast away a const or a volatile, that makes the compiler complains. If it works here that's nice but I wouldn't push to hard for this warning.

[dinosaure]

For clarity, I rebased this PR to one commit, the diff is really small. I think it's ok to merge. -Wcast-qual is a bit restrictive and ask us to use _Generic (C11) but it leads us to write good C code. I will merge it when the CI is green.

[MisterDA]

C++ has const_cast which could come in handy if there was an equivalent in C, when -Wcast-qual is enabled. I think it can be emulated with modern GCC/Clang with:

/* const_cast(ptr) can only be used inside functions, on pointers */

#if (defined(__GNUC__) && __GNUC__ >= 14) || \
    (defined(__clang_major__) && __clang_major__ >= 19)
#define const_cast(ptr)                               \
 ({                                                   \
    _Pragma("GCC diagnostic push")                    \
    _Pragma("GCC diagnostic ignored \"-Wcast-qual\"") \
    (__typeof_unqual__(*(ptr)) *) (ptr);              \
    _Pragma("GCC diagnostic pop")                     \
  })
#else
#define const_cast(ptr) (ptr)
#endif

int main(void) {
    const int * const p0 = (void *)0;
    int *p1 = const_cast(p0);
    return 0;
}

[dinosaure]

I think it's ok to merge now.

[hannesm]

I don't understand this change as part of this PR...

[hannesm]

wouldn't the same be to start from i = 1 in the for loop?

A comment why i=0 is special would be very welcome.