From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7C940A0547; Fri, 12 Mar 2021 12:46:49 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E9DD5406FF; Fri, 12 Mar 2021 12:46:48 +0100 (CET) Received: from dal3relay53.mxroute.com (dal3relay53.mxroute.com [64.40.27.53]) by mails.dpdk.org (Postfix) with ESMTP id 544864067E for ; Fri, 12 Mar 2021 12:46:47 +0100 (CET) Received: from filter004.mxroute.com ([149.28.56.236] filter004.mxroute.com) (Authenticated sender: mN4UYu2MZsgR) by dal3relay53.mxroute.com (ZoneMTA) with ESMTPSA id 17826429f58000a12d.001 for (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES128-GCM-SHA256); Fri, 12 Mar 2021 11:46:43 +0000 X-Zone-Loop: 1813095dff6668af66bb4fbef6c204f783edb3045ca4 X-Originating-IP: [149.28.56.236] DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=ashroe.eu; s=x; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:MIME-Version:Date: Message-ID:From:References:Cc:To:Subject:Sender:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=UTwZK1Slifiael/g5E11AlfbKF1/VSGRJwvZD54/mjQ=; b=Dg7TNJZiOHO1CAk7leyRwedG3+ M6I/Z9I5XjIP86MuKKOUh9lySE4R5LFhF/qOjBm4v8B49NB9L1LIz3gqTRiNkgBjS4VEcp905+QkG jwLXxqVM7B7GFhGAmiEIuHF9WL8nXUstBUNO3BeMjxLnChXmTtSzaTHhTV/CnWNvas3MsaAhTIxEJ WNGk31aPvkOE65Le6uFl2aabZT4LuCJ12WEF4lh8bcO+7vzj8asEgcqSRedCdybRj+Jb88Fx+QxAM 5JuqK3auN6Blii5NUldQbO2KZlFUIrEGDtuSTnYO61QU2ArWIfy89xUbXZ1rCPXoLO3ejW4WGGwsW Q4iOZb0w==; To: =?UTF-8?Q?Morten_Br=c3=b8rup?= , Tyler Retzlaff , Stephen Hemminger Cc: dev@dpdk.org, anatoly.burakov@intel.com, Neil Horman References: <1615358466-12761-1-git-send-email-roretzla@linux.microsoft.com> <20210310104942.66ef440e@hermes.local> <20210310225238.GA10267@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35C61674@smartserver.smartshare.dk> From: "Kinsella, Ray" Message-ID: Date: Fri, 12 Mar 2021 11:46:39 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35C61674@smartserver.smartshare.dk> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-AuthUser: mdr@ashroe.eu Subject: Re: [dpdk-dev] [PATCH] librte_eal/common: fix return type of rte_bsf64 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 12/03/2021 07:34, Morten Brørup wrote: > CC: ABI Policy maintainers. You might have an opinion. Or not. :-) My 2c is that this is affecting inlined functions from include/rte_common.h. Although not ideal, the practical effect on Binary Compatibility is therefore likely to be _nil_. Just a small comment - when Steve points out "The cast is no longer needed, it should be removed." I think Tyler _may_ have mis-understood his point when he said "it's not so much about making it compile. it's about making it correct with respect to original author intent, consistent with the rte_bsf32 declaration ... " My interpretation of what Steve said, is that once you change the return type to uint32_t ... there is no need for the cast any more, that is all. > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tyler Retzlaff >> Sent: Wednesday, March 10, 2021 11:53 PM >> >> On Wed, Mar 10, 2021 at 10:49:42AM -0800, Stephen Hemminger wrote: >>> On Tue, 9 Mar 2021 22:41:06 -0800 >>> Tyler Retzlaff wrote: >>> >>>> based on the original commit and the usage of rte_bsf64 it appears >> the >>>> function should always have returned uint32_t instead of int which >> is >>>> consistent with the cast introduced in the return statement. >>>> >>>> Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf >>>> functions") >>>> Cc: anatoly.burakov@intel.com >>>> >>>> Signed-off-by: Tyler Retzlaff >>>> --- >>>> lib/librte_eal/include/rte_common.h | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/lib/librte_eal/include/rte_common.h >> b/lib/librte_eal/include/rte_common.h >>>> index 1b630baf1..5e70ee7a8 100644 >>>> --- a/lib/librte_eal/include/rte_common.h >>>> +++ b/lib/librte_eal/include/rte_common.h >>>> @@ -679,7 +679,7 @@ rte_fls_u32(uint32_t x) >>>> * @return >>>> * least significant set bit in the input parameter. >>>> */ >>>> -static inline int >>>> +static inline uint32_t >>>> rte_bsf64(uint64_t v) >>>> { >>>> return (uint32_t)__builtin_ctzll(v); >>> >>> The cast is no longer needed, it should be removed. >> >> it's not so much about making it compile. it's about making it correct >> with respect to original author intent, consistent with the rte_bsf32 >> declaration, other consumers of the inline function inside rte_common.h >> and whether or not it makes sense to have a function that returns a >> count of bits signed. based on those factors i'm asserting that the >> cast is actually correct and it is the return type that is wrong. >> >> your suggestion however would avoid having to deal with the downside of >> changing the return type which is that an api change is necessary since >> the function is exposed to applications. but even for something small >> like this i think it is best to pursue the correct change rather than >> sprinkle casts to (uint32_t) at various call-sites. >> >> i'm in the process of sending the proposal to deprecate/change the >> return type unless others feel the above evaluation missed the mark. >> >> thanks! > > Please also update the similar math functions in rte_common.h, so the return type is consistent across these functions: > - rte_bsf32() > - rte_bsf32_safe() > - rte_fls_u32() > - rte_bsf64() > - rte_fls_u64() > - rte_log2_u32() > - rte_log2_u64() > > They should all return either int or uint32_t. > > Standard C conventions would have them all return int (probably due to C's default type promotion to int when used in calculations), which is also the type returned by their underlying implementation. > > For some unknown reason, DPDK often uses uint32_t where you would normally use int. I guess it was inspired by MISRA C (for embedded systems); but it is not a documented conventions, and often deviated from. > > I don't have a personal preference for int or uint32_t here. But at least follow the same convention in the same header file. > > (Please note that the functions returning a Boolean value as an int type should keep doing that.) > > > Med venlig hilsen / kind regards > - Morten Brørup >