From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier MATZ Subject: Re: [PATCH 1/5] rte_timer: fix invalid declaration of rte_timer_cb_t Date: Tue, 24 Feb 2015 13:36:04 +0100 Message-ID: <54EC7034.1030806@6wind.com> References: <1424700600-1765-1-git-send-email-pawelx.wodkowski@intel.com> <1424700600-1765-2-git-send-email-pawelx.wodkowski@intel.com> <54EC54C5.2060002@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit To: "Wodkowski, PawelX" , "dev-VfR2kkLFssw@public.gmane.org" Return-path: In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" Hi Pawel, On 02/24/2015 12:12 PM, Wodkowski, PawelX wrote: > > >> -----Original Message----- >> From: Olivier MATZ [mailto:olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org] >> Sent: Tuesday, February 24, 2015 11:39 AM >> To: Wodkowski, PawelX; dev-VfR2kkLFssw@public.gmane.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