Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

p_context pointers are declared as const pointers #93

Open
hwmaier opened this issue Jul 19, 2021 · 6 comments
Open

p_context pointers are declared as const pointers #93

hwmaier opened this issue Jul 19, 2021 · 6 comments

Comments

@hwmaier
Copy link
Contributor

hwmaier commented Jul 19, 2021

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.

@kjassmann-renesas
Copy link
Contributor

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.

@hwmaier
Copy link
Contributor Author

hwmaier commented Jul 21, 2021

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.

@kjassmann-renesas
Copy link
Contributor

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.

@hwmaier
Copy link
Contributor Author

hwmaier commented Jul 21, 2021

Great. The approach to change it for v4.0 makes sense,

@renesas-austin-hansen
Copy link
Collaborator

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.

@renesas-abigail
Copy link
Collaborator

This issue is tracked internally by SWFLEX-2991.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants