[dpdk-dev,1/5] rte_timer: fix invalid declaration of rte_timer_cb_t
Commit Message
Declaration for function pointer should be
typedef ret_type (*type_name)(args...)
not
typedef ret_type (type_name)(args...)
although compiler treat both of them the same, the static analysis tool
like klocwork complain about that.
Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
---
lib/librte_timer/rte_timer.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Comments
Hi Pawel,
On 02/23/2015 03:09 PM, Pawel Wodkowski wrote:
> Declaration for function pointer should be
> typedef ret_type (*type_name)(args...)
> not
> typedef ret_type (type_name)(args...)
>
> although compiler treat both of them the same, the static analysis tool
> like klocwork complain about that.
Can you give some details about the reason why klocwork is
complaining?
Looking at the C11 standard, it seems that this syntax is
legal. Please see EXAMPLE 4, page 156 of:
http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1570.pdf
Regards,
Olivier
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, February 24, 2015 11:39 AM
> To: Wodkowski, PawelX; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/5] rte_timer: fix invalid declaration of
> rte_timer_cb_t
>
> Hi Pawel,
>
> On 02/23/2015 03:09 PM, Pawel Wodkowski wrote:
> > Declaration for function pointer should be
> > typedef ret_type (*type_name)(args...)
> > not
> > typedef ret_type (type_name)(args...)
> >
> > although compiler treat both of them the same, the static analysis tool
> > like klocwork complain about that.
>
> Can you give some details about the reason why klocwork is
> complaining?
>
> Looking at the C11 standard, it seems that this syntax is
> legal. Please see EXAMPLE 4, page 156 of:
> http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1570.pdf
>
Legal, might be. Problem is in using it. In struct rte_timer field 'f' if
declared as pointer to rte_timer_cb_t but __rte_timer_reset() expects
rte_timer_cb_t. This have a little impact to real code but it is inconsistent
with declaration, definition and rest of the library where first syntax is used.
There are some places where second declaration is used but its usage there
is consistent with declaration.
I looked at the code in rest of library and for consistency I changed
typedef rather than function declaration.
Pawel
Hi Pawel,
On 02/24/2015 12:12 PM, Wodkowski, PawelX wrote:
>
>
>> -----Original Message-----
>> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
>> Sent: Tuesday, February 24, 2015 11:39 AM
>> To: Wodkowski, PawelX; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 1/5] rte_timer: fix invalid declaration of
>> rte_timer_cb_t
>>
>> Hi Pawel,
>>
>> On 02/23/2015 03:09 PM, Pawel Wodkowski wrote:
>>> Declaration for function pointer should be
>>> typedef ret_type (*type_name)(args...)
>>> not
>>> typedef ret_type (type_name)(args...)
>>>
>>> although compiler treat both of them the same, the static analysis tool
>>> like klocwork complain about that.
>>
>> Can you give some details about the reason why klocwork is
>> complaining?
>>
>> Looking at the C11 standard, it seems that this syntax is
>> legal. Please see EXAMPLE 4, page 156 of:
>> http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1570.pdf
>>
>
> Legal, might be. Problem is in using it. In struct rte_timer field 'f' if
> declared as pointer to rte_timer_cb_t but __rte_timer_reset() expects
> rte_timer_cb_t. This have a little impact to real code but it is inconsistent
> with declaration, definition and rest of the library where first syntax is used.
> There are some places where second declaration is used but its usage there
> is consistent with declaration.
>
> I looked at the code in rest of library and for consistency I changed
> typedef rather than function declaration.
OK, got it.
The problem was not the declaration itself but the inconsistency
between the function arguments and the rte_timer structure.
If you plan to do a v2 (maybe for patch 5/5), do you think you can
reword the commit log and title to clarify this a bit more?
Thanks,
Olivier
@@ -115,7 +115,7 @@ struct rte_timer;
/**
* Callback function type for timer expiry.
*/
-typedef void (rte_timer_cb_t)(struct rte_timer *, void *);
+typedef void (*rte_timer_cb_t)(struct rte_timer *, void *);
#define MAX_SKIPLIST_DEPTH 10
@@ -128,7 +128,7 @@ struct rte_timer
struct rte_timer *sl_next[MAX_SKIPLIST_DEPTH];
volatile union rte_timer_status status; /**< Status of timer. */
uint64_t period; /**< Period of timer (0 if not periodic). */
- rte_timer_cb_t *f; /**< Callback function. */
+ rte_timer_cb_t f; /**< Callback function. */
void *arg; /**< Argument to callback function. */
};