diff mbox

[dpdk-dev,v3,08/24] rte_ring_generic.h: stack declarations before code

Message ID 152609036246.121661.18385114679529023835.stgit@localhost.localdomain (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Andy Green May 12, 2018, 1:59 a.m. UTC 
  /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:
In function '__rte_ring_move_prod_head':
/projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:76:3:
warning: ISO C90 forbids mixed declarations and code
[-Wdeclaration-after-statement]
   const uint32_t cons_tail = r->cons.tail;
   ^~~~~
/projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:
In function '__rte_ring_move_cons_head':
/projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:147:3:
warning: ISO C90 forbids mixed declarations and code
[-Wdeclaration-after-statement]
   const uint32_t prod_tail = r->prod.tail;

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_ring/rte_ring_generic.h |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Thomas Monjalon May 13, 2018, 4:47 p.m. UTC | #1 
12/05/2018 03:59, Andy Green:
> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:
> In function '__rte_ring_move_prod_head':
> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:76:3:
> warning: ISO C90 forbids mixed declarations and code
> [-Wdeclaration-after-statement]
>    const uint32_t cons_tail = r->cons.tail;
>    ^~~~~
> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:
> In function '__rte_ring_move_cons_head':
> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:147:3:
> warning: ISO C90 forbids mixed declarations and code
> [-Wdeclaration-after-statement]
>    const uint32_t prod_tail = r->prod.tail;

The fix line is:
Fixes: 0dfc98c507b1 ("ring: separate out head index manipulation")

But I wonder why it was done like this. Maybe there is a hidden reason.
Bruce? Olivier?

> Signed-off-by: Andy Green <andy@warmcat.com>
[...]
> --- a/lib/librte_ring/rte_ring_generic.h
> +++ b/lib/librte_ring/rte_ring_generic.h
> @@ -73,14 +73,13 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp,
>  		 */
>  		rte_smp_rmb();
>  
> -		const uint32_t cons_tail = r->cons.tail;
>  		/*
>  		 *  The subtraction is done between two unsigned 32bits value
>  		 * (the result is always modulo 32 bits even if we have
>  		 * *old_head > cons_tail). So 'free_entries' is always between 0
>  		 * and capacity (which is < size).
>  		 */
> -		*free_entries = (capacity + cons_tail - *old_head);
> +		*free_entries = (capacity + r->cons.tail - *old_head);
Andy Green May 13, 2018, 11:21 p.m. UTC | #2 
On 05/14/2018 12:47 AM, Thomas Monjalon wrote:
> 12/05/2018 03:59, Andy Green:
>> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:
>> In function '__rte_ring_move_prod_head':
>> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:76:3:
>> warning: ISO C90 forbids mixed declarations and code
>> [-Wdeclaration-after-statement]
>>     const uint32_t cons_tail = r->cons.tail;
>>     ^~~~~
>> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:
>> In function '__rte_ring_move_cons_head':
>> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:147:3:
>> warning: ISO C90 forbids mixed declarations and code
>> [-Wdeclaration-after-statement]
>>     const uint32_t prod_tail = r->prod.tail;
> 
> The fix line is:
> Fixes: 0dfc98c507b1 ("ring: separate out head index manipulation")

OK... people can just use git blame at the time they want to know this 
though.

> But I wonder why it was done like this. Maybe there is a hidden reason.
> Bruce? Olivier?

It's not crazy to do it for clarity, or when you thought there'd be more 
uses of the calculation.  At any rate, I don't see any openings for it 
to affect the code changing it.

-Andy

>> Signed-off-by: Andy Green <andy@warmcat.com>
> [...]
>> --- a/lib/librte_ring/rte_ring_generic.h
>> +++ b/lib/librte_ring/rte_ring_generic.h
>> @@ -73,14 +73,13 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp,
>>   		 */
>>   		rte_smp_rmb();
>>   
>> -		const uint32_t cons_tail = r->cons.tail;
>>   		/*
>>   		 *  The subtraction is done between two unsigned 32bits value
>>   		 * (the result is always modulo 32 bits even if we have
>>   		 * *old_head > cons_tail). So 'free_entries' is always between 0
>>   		 * and capacity (which is < size).
>>   		 */
>> -		*free_entries = (capacity + cons_tail - *old_head);
>> +		*free_entries = (capacity + r->cons.tail - *old_head);
> 
> 
>
Thomas Monjalon May 13, 2018, 11:44 p.m. UTC | #3 
14/05/2018 01:21, Andy Green:
> 
> On 05/14/2018 12:47 AM, Thomas Monjalon wrote:
> > 12/05/2018 03:59, Andy Green:
> >> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:
> >> In function '__rte_ring_move_prod_head':
> >> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:76:3:
> >> warning: ISO C90 forbids mixed declarations and code
> >> [-Wdeclaration-after-statement]
> >>     const uint32_t cons_tail = r->cons.tail;
> >>     ^~~~~
> >> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:
> >> In function '__rte_ring_move_cons_head':
> >> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:147:3:
> >> warning: ISO C90 forbids mixed declarations and code
> >> [-Wdeclaration-after-statement]
> >>     const uint32_t prod_tail = r->prod.tail;
> > 
> > The fix line is:
> > Fixes: 0dfc98c507b1 ("ring: separate out head index manipulation")
> 
> OK... people can just use git blame at the time they want to know this 
> though.

No, we need it to automatically detect which commits should be backported,
and in which branch.

We also add Cc: stable@dpdk.org to confirm explicitly that is must be backported.
Andy Green May 14, 2018, 12:09 a.m. UTC | #4 
On 05/14/2018 07:44 AM, Thomas Monjalon wrote:
> 14/05/2018 01:21, Andy Green:
>>
>> On 05/14/2018 12:47 AM, Thomas Monjalon wrote:
>>> 12/05/2018 03:59, Andy Green:
>>>> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:
>>>> In function '__rte_ring_move_prod_head':
>>>> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:76:3:
>>>> warning: ISO C90 forbids mixed declarations and code
>>>> [-Wdeclaration-after-statement]
>>>>      const uint32_t cons_tail = r->cons.tail;
>>>>      ^~~~~
>>>> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:
>>>> In function '__rte_ring_move_cons_head':
>>>> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:147:3:
>>>> warning: ISO C90 forbids mixed declarations and code
>>>> [-Wdeclaration-after-statement]
>>>>      const uint32_t prod_tail = r->prod.tail;
>>>
>>> The fix line is:
>>> Fixes: 0dfc98c507b1 ("ring: separate out head index manipulation")
>>
>> OK... people can just use git blame at the time they want to know this
>> though.
> 
> No, we need it to automatically detect which commits should be backported,

There is nothing automatic about that flow :-)

It's "automatic after every committer did the work manually" you mean.

> and in which branch.
> 
> We also add Cc: stable@dpdk.org to confirm explicitly that is must be backported.

In other projects this work falls on the maintainer(s).

-Andy

> 
>
Thomas Monjalon May 14, 2018, 1:49 a.m. UTC | #5 
14/05/2018 02:09, Andy Green:
> 
> On 05/14/2018 07:44 AM, Thomas Monjalon wrote:
> > 14/05/2018 01:21, Andy Green:
> >>
> >> On 05/14/2018 12:47 AM, Thomas Monjalon wrote:
> >>> 12/05/2018 03:59, Andy Green:
> >>>> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:
> >>>> In function '__rte_ring_move_prod_head':
> >>>> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:76:3:
> >>>> warning: ISO C90 forbids mixed declarations and code
> >>>> [-Wdeclaration-after-statement]
> >>>>      const uint32_t cons_tail = r->cons.tail;
> >>>>      ^~~~~
> >>>> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:
> >>>> In function '__rte_ring_move_cons_head':
> >>>> /projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:147:3:
> >>>> warning: ISO C90 forbids mixed declarations and code
> >>>> [-Wdeclaration-after-statement]
> >>>>      const uint32_t prod_tail = r->prod.tail;
> >>>
> >>> The fix line is:
> >>> Fixes: 0dfc98c507b1 ("ring: separate out head index manipulation")
> >>
> >> OK... people can just use git blame at the time they want to know this
> >> though.
> > 
> > No, we need it to automatically detect which commits should be backported,
> 
> There is nothing automatic about that flow :-)
> 
> It's "automatic after every committer did the work manually" you mean.

Yes.
In this case, I found it for you.
You just need to keep it if doing a new revision.

> > and in which branch.
> > 
> > We also add Cc: stable@dpdk.org to confirm explicitly that is must be backported.
> 
> In other projects this work falls on the maintainer(s).

In DPDK too, the maintainers often fill this field
(I did it for your 8 patches just pushed today).
But we prefer when this work is done by the patch author :)
diff mbox

Patch

diff --git a/lib/librte_ring/rte_ring_generic.h b/lib/librte_ring/rte_ring_generic.h
index 5b110425f..c2d482bc9 100644
--- a/lib/librte_ring/rte_ring_generic.h
+++ b/lib/librte_ring/rte_ring_generic.h
@@ -73,14 +73,13 @@  __rte_ring_move_prod_head(struct rte_ring *r, int is_sp,
 		 */
 		rte_smp_rmb();
 
-		const uint32_t cons_tail = r->cons.tail;
 		/*
 		 *  The subtraction is done between two unsigned 32bits value
 		 * (the result is always modulo 32 bits even if we have
 		 * *old_head > cons_tail). So 'free_entries' is always between 0
 		 * and capacity (which is < size).
 		 */
-		*free_entries = (capacity + cons_tail - *old_head);
+		*free_entries = (capacity + r->cons.tail - *old_head);
 
 		/* check that we have enough room in ring */
 		if (unlikely(n > *free_entries))
@@ -144,13 +143,12 @@  __rte_ring_move_cons_head(struct rte_ring *r, int is_sc,
 		 */
 		rte_smp_rmb();
 
-		const uint32_t prod_tail = r->prod.tail;
 		/* The subtraction is done between two unsigned 32bits value
 		 * (the result is always modulo 32 bits even if we have
 		 * cons_head > prod_tail). So 'entries' is always between 0
 		 * and size(ring)-1.
 		 */
-		*entries = (prod_tail - *old_head);
+		*entries = (r->prod.tail - *old_head);
 
 		/* Set the actual entries for dequeue */
 		if (n > *entries)