diff mbox series

eal, power: don't use '-' sign with unsigned literals

Message ID 1615333490-15243-1-git-send-email-roretzla@linux.microsoft.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series eal, power: don't use '-' sign with unsigned literals | expand

Checks

Context Check Description
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Functional success Functional Testing PASS
ci/iol-testing fail Testing issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/github-robot success github build: passed
ci/travis-robot success travis build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Tyler Retzlaff March 9, 2021, 11:44 p.m. UTC
use ~0ULL instead of -1ULL to avoid contridctory application of '-' sign
to integer literal where the desired type is unsigned.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/librte_eal/common/eal_common_fbarray.c | 12 ++++++------
 lib/librte_power/rte_power_pmd_mgmt.c      |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

Comments

Burakov, Anatoly March 12, 2021, 12:51 p.m. UTC | #1
On 09-Mar-21 11:44 PM, Tyler Retzlaff wrote:
> use ~0ULL instead of -1ULL to avoid contridctory application of '-' sign
> to integer literal where the desired type is unsigned.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---

Not sure i agree. It's a very common pattern and is widely used and 
understood. I mean, if anything, seeing `~0` would have me stop and 
think as i've literally never seen such code before.
Tyler Retzlaff March 12, 2021, 5:05 p.m. UTC | #2
On Fri, Mar 12, 2021 at 12:51:46PM +0000, Burakov, Anatoly wrote:
> On 09-Mar-21 11:44 PM, Tyler Retzlaff wrote:
> >use ~0ULL instead of -1ULL to avoid contridctory application of '-' sign
> >to integer literal where the desired type is unsigned.
> >
> >Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> >---
> 
> Not sure i agree. It's a very common pattern and is widely used and
> understood. I mean, if anything, seeing `~0` would have me stop and
> think as i've literally never seen such code before.

it produces warnings under some compilers. in some enterprises we are
required to fix certain classes of warnings (not suppress them from the
command line) as a function of security policies.

as an alternative would you be more willing to accept something like the
following? ``(unsigned long long)-1LL'' if you don't like ``~0ULL'' it
would make explicit what the compiler is already doing.

the issue is the application of the sign to what is clearly something not
signed; it get's flagged. so the cast is an explicit expression of intent
that will not generate the warnings.

appreciate you're help in finding a solution even if it isn't the
proposed solution.

thanks!

> 
> -- 
> Thanks,
> Anatoly
Bruce Richardson March 12, 2021, 5:37 p.m. UTC | #3
On Fri, Mar 12, 2021 at 09:05:58AM -0800, Tyler Retzlaff wrote:
> On Fri, Mar 12, 2021 at 12:51:46PM +0000, Burakov, Anatoly wrote:
> > On 09-Mar-21 11:44 PM, Tyler Retzlaff wrote:
> > >use ~0ULL instead of -1ULL to avoid contridctory application of '-' sign
> > >to integer literal where the desired type is unsigned.
> > >
> > >Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > >---
> > 
> > Not sure i agree. It's a very common pattern and is widely used and
> > understood. I mean, if anything, seeing `~0` would have me stop and
> > think as i've literally never seen such code before.
> 
> it produces warnings under some compilers. in some enterprises we are
> required to fix certain classes of warnings (not suppress them from the
> command line) as a function of security policies.
> 
> as an alternative would you be more willing to accept something like the
> following? ``(unsigned long long)-1LL'' if you don't like ``~0ULL'' it
> would make explicit what the compiler is already doing.
> 
> the issue is the application of the sign to what is clearly something not
> signed; it get's flagged. so the cast is an explicit expression of intent
> that will not generate the warnings.
> 
> appreciate you're help in finding a solution even if it isn't the
> proposed solution.
> 
What about using ULLONG_MAX and similar defines from limits.h?
Tyler Retzlaff March 12, 2021, 6:36 p.m. UTC | #4
On Fri, Mar 12, 2021 at 05:37:41PM +0000, Bruce Richardson wrote:
> On Fri, Mar 12, 2021 at 09:05:58AM -0800, Tyler Retzlaff wrote:
> > > Not sure i agree. It's a very common pattern and is widely used and
> > > understood. I mean, if anything, seeing `~0` would have me stop and
> > > think as i've literally never seen such code before.
> > 
> > it produces warnings under some compilers. in some enterprises we are
> > required to fix certain classes of warnings (not suppress them from the
> > command line) as a function of security policies.
> > 
> > as an alternative would you be more willing to accept something like the
> > following? ``(unsigned long long)-1LL'' if you don't like ``~0ULL'' it
> > would make explicit what the compiler is already doing.
> > 
> > the issue is the application of the sign to what is clearly something not
> > signed; it get's flagged. so the cast is an explicit expression of intent
> > that will not generate the warnings.
> > 
> > appreciate you're help in finding a solution even if it isn't the
> > proposed solution.
> > 
> What about using ULLONG_MAX and similar defines from limits.h?

i think this would be okay even in circumstances where the code is
building masks so long as in practice it results in "all bits being
set". i'm not aware of a XXX_MAX where max isn't all bits set.. is
there?
Tyler Retzlaff March 12, 2021, 6:40 p.m. UTC | #5
On Fri, Mar 12, 2021 at 10:36:15AM -0800, Tyler Retzlaff wrote:
> On Fri, Mar 12, 2021 at 05:37:41PM +0000, Bruce Richardson wrote:
> > On Fri, Mar 12, 2021 at 09:05:58AM -0800, Tyler Retzlaff wrote:
> > > > Not sure i agree. It's a very common pattern and is widely used and
> > > > understood. I mean, if anything, seeing `~0` would have me stop and
> > > > think as i've literally never seen such code before.
> > > 
> > > it produces warnings under some compilers. in some enterprises we are
> > > required to fix certain classes of warnings (not suppress them from the
> > > command line) as a function of security policies.
> > > 
> > > as an alternative would you be more willing to accept something like the
> > > following? ``(unsigned long long)-1LL'' if you don't like ``~0ULL'' it
> > > would make explicit what the compiler is already doing.
> > > 
> > > the issue is the application of the sign to what is clearly something not
> > > signed; it get's flagged. so the cast is an explicit expression of intent
> > > that will not generate the warnings.
> > > 
> > > appreciate you're help in finding a solution even if it isn't the
> > > proposed solution.
> > > 
> > What about using ULLONG_MAX and similar defines from limits.h?
> 
> i think this would be okay even in circumstances where the code is
> building masks so long as in practice it results in "all bits being
> set". i'm not aware of a XXX_MAX where max isn't all bits set.. is
> there?

just a qualification to my previous.

specifically for the UXXX_MAX (unsigned) preprocessor definitions, we
aren't talking about signed here (or at least i wasn't).
Morten Brørup March 13, 2021, 9:05 a.m. UTC | #6
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tyler Retzlaff
> Sent: Friday, March 12, 2021 7:40 PM
> 
> On Fri, Mar 12, 2021 at 10:36:15AM -0800, Tyler Retzlaff wrote:
> > On Fri, Mar 12, 2021 at 05:37:41PM +0000, Bruce Richardson wrote:
> > > On Fri, Mar 12, 2021 at 09:05:58AM -0800, Tyler Retzlaff wrote:
> > > > > Not sure i agree. It's a very common pattern and is widely used and
> > > > > understood. I mean, if anything, seeing `~0` would have me stop and
> > > > > think as i've literally never seen such code before.
> > > >
> > > > it produces warnings under some compilers. in some enterprises we are
> > > > required to fix certain classes of warnings (not suppress them from
> the
> > > > command line) as a function of security policies.
> > > >
> > > > as an alternative would you be more willing to accept something like
> the
> > > > following? ``(unsigned long long)-1LL'' if you don't like ``~0ULL''
> it
> > > > would make explicit what the compiler is already doing.
> > > >
> > > > the issue is the application of the sign to what is clearly something
> not
> > > > signed; it get's flagged. so the cast is an explicit expression of
> intent
> > > > that will not generate the warnings.
> > > >
> > > > appreciate you're help in finding a solution even if it isn't the
> > > > proposed solution.
> > > >
> > > What about using ULLONG_MAX and similar defines from limits.h?

The type of the variable being manipulated is not unsigned long long, but uint64_t, which theoretically is not the same. Long long can be more than 64 bit, although all current implementations use 64 bit for long long.

So -1ULL was wrong from the start, and ~0ULL is similarly incorrect. The correct value would be UINT64_MAX from stdint.h.

> >
> > i think this would be okay even in circumstances where the code is
> > building masks so long as in practice it results in "all bits being
> > set". i'm not aware of a XXX_MAX where max isn't all bits set.. is
> > there?
> 
> just a qualification to my previous.
> 
> specifically for the UXXX_MAX (unsigned) preprocessor definitions, we
> aren't talking about signed here (or at least i wasn't).
diff mbox series

Patch

diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c
index 592ec5859..82d271725 100644
--- a/lib/librte_eal/common/eal_common_fbarray.c
+++ b/lib/librte_eal/common/eal_common_fbarray.c
@@ -138,7 +138,7 @@  find_next_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
 	 */
 	last = MASK_LEN_TO_IDX(arr->len);
 	last_mod = MASK_LEN_TO_MOD(arr->len);
-	last_msk = ~(-1ULL << last_mod);
+	last_msk = ~(~0ULL << last_mod);
 
 	for (msk_idx = first; msk_idx < msk->n_masks; msk_idx++) {
 		uint64_t cur_msk, lookahead_msk;
@@ -398,8 +398,8 @@  find_prev_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
 	first_mod = MASK_LEN_TO_MOD(start);
 	/* we're going backwards, so mask must start from the top */
 	ignore_msk = first_mod == MASK_ALIGN - 1 ?
-				-1ULL : /* prevent overflow */
-				~(-1ULL << (first_mod + 1));
+				~0ULL : /* prevent overflow */
+				~(~0ULL << (first_mod + 1));
 
 	/* go backwards, include zero */
 	msk_idx = first;
@@ -513,7 +513,7 @@  find_prev_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
 				 * no runs in the space we've lookbehind-scanned
 				 * as well, so skip that on next iteration.
 				 */
-				ignore_msk = -1ULL << need;
+				ignore_msk = ~0ULL << need;
 				msk_idx = lookbehind_idx;
 				break;
 			}
@@ -560,8 +560,8 @@  find_prev(const struct rte_fbarray *arr, unsigned int start, bool used)
 	first_mod = MASK_LEN_TO_MOD(start);
 	/* we're going backwards, so mask must start from the top */
 	ignore_msk = first_mod == MASK_ALIGN - 1 ?
-				-1ULL : /* prevent overflow */
-				~(-1ULL << (first_mod + 1));
+				~0ULL : /* prevent overflow */
+				~(~0ULL << (first_mod + 1));
 
 	/* go backwards, include zero */
 	idx = first;
diff --git a/lib/librte_power/rte_power_pmd_mgmt.c b/lib/librte_power/rte_power_pmd_mgmt.c
index 454ef7091..579f10e4b 100644
--- a/lib/librte_power/rte_power_pmd_mgmt.c
+++ b/lib/librte_power/rte_power_pmd_mgmt.c
@@ -111,7 +111,7 @@  clb_umwait(uint16_t port_id, uint16_t qidx, struct rte_mbuf **pkts __rte_unused,
 				ret = rte_eth_get_monitor_addr(port_id, qidx,
 						&pmc);
 				if (ret == 0)
-					rte_power_monitor(&pmc, -1ULL);
+					rte_power_monitor(&pmc, ~0ULL);
 			}
 			q_conf->umwait_in_progress = false;