fsp
fsp copied to clipboard
renesas
p_context pointers are declared as const pointers
What is the rationale to use a const pointer rather a normal void pointer for p_context fields?
According to the documentation this pointer is intended to hold user data and make it available to callback functions. Declaring this pointer as const means that the user data it holds cannot be modified from within the callback function.
This limits its use and while in C the const can be easily cast away using a type cast, in C++ this requires unnecessary const_casts to remove the constness.
I suggest to change p_context to a simple void * pointer so it can hold both const and non-const user data.
In FSP, p_context is user input data, and the type it points to cannot be known. It is pointer-to-const type to limit compiler warnings/errors due to discarded const qualifier.
These warnings/errors show up in all the C compilers we support if the const qualifier is removed and a pointer-to-const is used to initialize p_context without casting.
There are no warnings/errors from our C compilers if a normal pointer is used to initialize p_context as written without casting.
If the pointer constness differ, then either there is either a warning when assigning to p_context or a warning when using p_context in the callback function. This would be "initialization discards 'const' qualifier from pointer target type" with C or "invalid conversion from 'const void*' to 'void*' [-fpermissive]" with C++.
As I would have thought the common use case for p_context is to point to non-const user data, something like
struct MyContext
{
int counter;
};
it would make more sense to have p_context as non-const pointer as then for the common use case no cast is required or warning appears.
Example what I mean, using the FSP can_fd_ek_ra6m5_ep example project:
No warning here in hal_entry.c:
struct MyContext
{
int counter;
};
struct MyContext myContext;
void hal_entry(void)
{
...
g_canfd0_ctrl.p_context = &myContext;
err = R_CANFD_Open(&g_canfd0_ctrl, &g_canfd0_cfg);
...
But here in can_fd_ep.c:
void canfd0_callback(can_callback_args_t *p_args)
{
struct MyContext* myPtr = p_args->p_context; // warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
myPtr->counter++;
...
Above example will compile without warnings if the p_context pointer is changed to void* type.
Of course my suggestion to change the type is based on the assumption the most common usage is to point to non-const data.
I agree the most common use case is for p_context to point to non-const data. Changing this would be a compatibility break since it will introduce build errors for IAR. We cannot break compatibility in minor releases, so we will consider this for 4.0.0.
Due to issues with many internal test configurations this relatively small change ended up taking more time than anticipated. Unfortunately, it did not make it in time for the v4.0.0 release. We will try to see if we can release it sooner than v5.0.0 if at all possible.
