diff mbox

[PATCHv2] validation: odp_timer: cleanup for clean termination

Message ID 1425548835-5105-1-git-send-email-ola.liljedahl@linaro.org
State New
Headers show

Commit Message

Ola Liljedahl March 5, 2015, 9:47 a.m. UTC 
Free queue and timeouts, destroy timeout pool before termination.
https://bugs.linaro.org/show_bug.cgi?id=1285

Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
---
(This document/code contribution attached is provided under the terms of
agreement LES-LTM-21309)

Removed reference to Linaro CARDS as this is internal.

 test/validation/odp_timer.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Bill Fischofer March 5, 2015, 3:57 p.m. UTC | #1 
On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl <ola.liljedahl@linaro.org>
wrote:

> Free queue and timeouts, destroy timeout pool before termination.
> https://bugs.linaro.org/show_bug.cgi?id=1285
>
> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
>

Reviewed-and-tested-by: Bill Fischofer <bill.fischofer@linaro.org>


> ---
> (This document/code contribution attached is provided under the terms of
> agreement LES-LTM-21309)
>
> Removed reference to Linaro CARDS as this is internal.
>
>  test/validation/odp_timer.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c
> index 88af61b..3e83367 100644
> --- a/test/validation/odp_timer.c
> +++ b/test/validation/odp_timer.c
> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void *arg)
>         if (ev != ODP_EVENT_INVALID)
>                 CU_FAIL("Unexpected event received");
>
> +       odp_queue_destroy(queue);
> +       for (i = 0; i < NTIMERS; i++) {
> +               if (tt[i].ev != ODP_EVENT_INVALID)
> +                       odp_timeout_free(odp_timeout_from_event(tt[i].ev));
> +       }
>         LOG_DBG("Thread %u: exiting\n", thr);
>         return NULL;
>  }
> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>         /* Destroy timer pool, all timers must have been freed */
>         odp_timer_pool_destroy(tp);
>
> +       /* Destroy timeout pool, all timeouts must have been freed */
> +       int rc = odp_pool_destroy(tbp);
> +       CU_ASSERT(rc == 0);
> +
>         CU_PASS("ODP timer test");
>  }
>
> --
> 1.9.1
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
Mike Holmes March 5, 2015, 6:57 p.m. UTC | #2 
On 5 March 2015 at 10:57, Bill Fischofer <bill.fischofer@linaro.org> wrote:

>
>
> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl <ola.liljedahl@linaro.org>
> wrote:
>
>> Free queue and timeouts, destroy timeout pool before termination.
>> https://bugs.linaro.org/show_bug.cgi?id=1285
>>
>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
>>
>
> Reviewed-and-tested-by: Bill Fischofer <bill.fischofer@linaro.org>
>
>
>> ---
>> (This document/code contribution attached is provided under the terms of
>> agreement LES-LTM-21309)
>>
>> Removed reference to Linaro CARDS as this is internal.
>>
>>  test/validation/odp_timer.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c
>> index 88af61b..3e83367 100644
>> --- a/test/validation/odp_timer.c
>> +++ b/test/validation/odp_timer.c
>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void *arg)
>>         if (ev != ODP_EVENT_INVALID)
>>                 CU_FAIL("Unexpected event received");
>>
>> +       odp_queue_destroy(queue);
>> +       for (i = 0; i < NTIMERS; i++) {
>> +               if (tt[i].ev != ODP_EVENT_INVALID)
>> +
>>  odp_timeout_free(odp_timeout_from_event(tt[i].ev));
>> +       }
>>         LOG_DBG("Thread %u: exiting\n", thr);
>>         return NULL;
>>  }
>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>>         /* Destroy timer pool, all timers must have been freed */
>>         odp_timer_pool_destroy(tp);
>>
>
Still need to set
(void)  odp_timer_pool_destroy(tp);
or
check the return code.
Else where in this file the explicit intention to ignore a return code is
signaled with (void)



>
>> +       /* Destroy timeout pool, all timeouts must have been freed */
>> +       int rc = odp_pool_destroy(tbp);
>> +       CU_ASSERT(rc == 0);
>> +
>>         CU_PASS("ODP timer test");
>>  }
>>
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Mike Holmes March 5, 2015, 6:58 p.m. UTC | #3 
pasted to wrong on this v2 - meant odp_queue_destroy

On 5 March 2015 at 13:57, Mike Holmes <mike.holmes@linaro.org> wrote:

>
>
> On 5 March 2015 at 10:57, Bill Fischofer <bill.fischofer@linaro.org>
> wrote:
>
>>
>>
>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl <ola.liljedahl@linaro.org>
>> wrote:
>>
>>> Free queue and timeouts, destroy timeout pool before termination.
>>> https://bugs.linaro.org/show_bug.cgi?id=1285
>>>
>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
>>>
>>
>> Reviewed-and-tested-by: Bill Fischofer <bill.fischofer@linaro.org>
>>
>>
>>> ---
>>> (This document/code contribution attached is provided under the terms of
>>> agreement LES-LTM-21309)
>>>
>>> Removed reference to Linaro CARDS as this is internal.
>>>
>>>  test/validation/odp_timer.c | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c
>>> index 88af61b..3e83367 100644
>>> --- a/test/validation/odp_timer.c
>>> +++ b/test/validation/odp_timer.c
>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void *arg)
>>>         if (ev != ODP_EVENT_INVALID)
>>>                 CU_FAIL("Unexpected event received");
>>>
>>> +       odp_queue_destroy(queue);
>>> +       for (i = 0; i < NTIMERS; i++) {
>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
>>> +
>>>  odp_timeout_free(odp_timeout_from_event(tt[i].ev));
>>> +       }
>>>         LOG_DBG("Thread %u: exiting\n", thr);
>>>         return NULL;
>>>  }
>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>>>         /* Destroy timer pool, all timers must have been freed */
>>>         odp_timer_pool_destroy(tp);
>>>
>>
> Still need to set
> (void)  odp_timer_pool_destroy(tp);
> or
> check the return code.
> Else where in this file the explicit intention to ignore a return code is
> signaled with (void)
>
>
>
>>
>>> +       /* Destroy timeout pool, all timeouts must have been freed */
>>> +       int rc = odp_pool_destroy(tbp);
>>> +       CU_ASSERT(rc == 0);
>>> +
>>>         CU_PASS("ODP timer test");
>>>  }
>>>
>>> --
>>> 1.9.1
>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>
>
> --
> Mike Holmes
> Technical Manager - Linaro Networking Group
> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs
>
>
>
Ola Liljedahl March 6, 2015, 9:57 a.m. UTC | #4 
OK fixed this in v3.

If this is a style rule (and I approve of it, I just didn't expect
odp_queue_destroy() to have any return value, I thought it was succeed
or crash semantics like I prefer), wouldn't it be good it there was
some way the compiler could always warn for this?
GCC states: "The default is -Wunused-result" but it seems like the
function needs to be declared with the "warn_unused_result" attribute.
Is there any way to get this warning for all functions? If not,
perhaps we should add this function attribute to ODP functions such as
odp_queue_destroy()?

On 5 March 2015 at 19:58, Mike Holmes <mike.holmes@linaro.org> wrote:
> pasted to wrong on this v2 - meant odp_queue_destroy
>
> On 5 March 2015 at 13:57, Mike Holmes <mike.holmes@linaro.org> wrote:
>>
>>
>>
>> On 5 March 2015 at 10:57, Bill Fischofer <bill.fischofer@linaro.org>
>> wrote:
>>>
>>>
>>>
>>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl <ola.liljedahl@linaro.org>
>>> wrote:
>>>>
>>>> Free queue and timeouts, destroy timeout pool before termination.
>>>> https://bugs.linaro.org/show_bug.cgi?id=1285
>>>>
>>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
>>>
>>>
>>> Reviewed-and-tested-by: Bill Fischofer <bill.fischofer@linaro.org>
>>>
>>>>
>>>> ---
>>>> (This document/code contribution attached is provided under the terms of
>>>> agreement LES-LTM-21309)
>>>>
>>>> Removed reference to Linaro CARDS as this is internal.
>>>>
>>>>  test/validation/odp_timer.c | 9 +++++++++
>>>>  1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c
>>>> index 88af61b..3e83367 100644
>>>> --- a/test/validation/odp_timer.c
>>>> +++ b/test/validation/odp_timer.c
>>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void *arg)
>>>>         if (ev != ODP_EVENT_INVALID)
>>>>                 CU_FAIL("Unexpected event received");
>>>>
>>>> +       odp_queue_destroy(queue);
>>>> +       for (i = 0; i < NTIMERS; i++) {
>>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
>>>> +
>>>> odp_timeout_free(odp_timeout_from_event(tt[i].ev));
>>>> +       }
>>>>         LOG_DBG("Thread %u: exiting\n", thr);
>>>>         return NULL;
>>>>  }
>>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>>>>         /* Destroy timer pool, all timers must have been freed */
>>>>         odp_timer_pool_destroy(tp);
>>
>>
>> Still need to set
>> (void)  odp_timer_pool_destroy(tp);
>> or
>> check the return code.
>> Else where in this file the explicit intention to ignore a return code is
>> signaled with (void)
>>
>>
>>>>
>>>>
>>>> +       /* Destroy timeout pool, all timeouts must have been freed */
>>>> +       int rc = odp_pool_destroy(tbp);
>>>> +       CU_ASSERT(rc == 0);
>>>> +
>>>>         CU_PASS("ODP timer test");
>>>>  }
>>>>
>>>> --
>>>> 1.9.1
>>>>
>>>>
>>>> _______________________________________________
>>>> lng-odp mailing list
>>>> lng-odp@lists.linaro.org
>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>
>>
>>
>> --
>> Mike Holmes
>> Technical Manager - Linaro Networking Group
>> Linaro.org │ Open source software for ARM SoCs
>>
>>
>
>
>
> --
> Mike Holmes
> Technical Manager - Linaro Networking Group
> Linaro.org │ Open source software for ARM SoCs
>
>
Mike Holmes March 6, 2015, 1:31 p.m. UTC | #5 
On 6 March 2015 at 04:57, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:

> OK fixed this in v3.
>
> If this is a style rule (and I approve of it, I just didn't expect
> odp_queue_destroy() to have any return value, I thought it was succeed
> or crash semantics like I prefer), wouldn't it be good it there was
> some way the compiler could always warn for this?
> GCC states: "The default is -Wunused-result" but it seems like the
> function needs to be declared with the "warn_unused_result" attribute.
> Is there any way to get this warning for all functions? If not,
> perhaps we should add this function attribute to ODP functions such as
> odp_queue_destroy()?
>
>
I am all for adding such hints, I am going to play with it and see how it
pans out.
If adding (void) suppress the warning in an application when you truly
don't want to look at the return code I see no harm in adding it to the
whole API.




> On 5 March 2015 at 19:58, Mike Holmes <mike.holmes@linaro.org> wrote:
> > pasted to wrong on this v2 - meant odp_queue_destroy
> >
> > On 5 March 2015 at 13:57, Mike Holmes <mike.holmes@linaro.org> wrote:
> >>
> >>
> >>
> >> On 5 March 2015 at 10:57, Bill Fischofer <bill.fischofer@linaro.org>
> >> wrote:
> >>>
> >>>
> >>>
> >>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl <
> ola.liljedahl@linaro.org>
> >>> wrote:
> >>>>
> >>>> Free queue and timeouts, destroy timeout pool before termination.
> >>>> https://bugs.linaro.org/show_bug.cgi?id=1285
> >>>>
> >>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
> >>>
> >>>
> >>> Reviewed-and-tested-by: Bill Fischofer <bill.fischofer@linaro.org>
> >>>
> >>>>
> >>>> ---
> >>>> (This document/code contribution attached is provided under the terms
> of
> >>>> agreement LES-LTM-21309)
> >>>>
> >>>> Removed reference to Linaro CARDS as this is internal.
> >>>>
> >>>>  test/validation/odp_timer.c | 9 +++++++++
> >>>>  1 file changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c
> >>>> index 88af61b..3e83367 100644
> >>>> --- a/test/validation/odp_timer.c
> >>>> +++ b/test/validation/odp_timer.c
> >>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void *arg)
> >>>>         if (ev != ODP_EVENT_INVALID)
> >>>>                 CU_FAIL("Unexpected event received");
> >>>>
> >>>> +       odp_queue_destroy(queue);
> >>>> +       for (i = 0; i < NTIMERS; i++) {
> >>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
> >>>> +
> >>>> odp_timeout_free(odp_timeout_from_event(tt[i].ev));
> >>>> +       }
> >>>>         LOG_DBG("Thread %u: exiting\n", thr);
> >>>>         return NULL;
> >>>>  }
> >>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
> >>>>         /* Destroy timer pool, all timers must have been freed */
> >>>>         odp_timer_pool_destroy(tp);
> >>
> >>
> >> Still need to set
> >> (void)  odp_timer_pool_destroy(tp);
> >> or
> >> check the return code.
> >> Else where in this file the explicit intention to ignore a return code
> is
> >> signaled with (void)
> >>
> >>
> >>>>
> >>>>
> >>>> +       /* Destroy timeout pool, all timeouts must have been freed */
> >>>> +       int rc = odp_pool_destroy(tbp);
> >>>> +       CU_ASSERT(rc == 0);
> >>>> +
> >>>>         CU_PASS("ODP timer test");
> >>>>  }
> >>>>
> >>>> --
> >>>> 1.9.1
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> lng-odp mailing list
> >>>> lng-odp@lists.linaro.org
> >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
> >>>
> >>>
> >>>
> >>> _______________________________________________
> >>> lng-odp mailing list
> >>> lng-odp@lists.linaro.org
> >>> http://lists.linaro.org/mailman/listinfo/lng-odp
> >>>
> >>
> >>
> >>
> >> --
> >> Mike Holmes
> >> Technical Manager - Linaro Networking Group
> >> Linaro.org │ Open source software for ARM SoCs
> >>
> >>
> >
> >
> >
> > --
> > Mike Holmes
> > Technical Manager - Linaro Networking Group
> > Linaro.org │ Open source software for ARM SoCs
> >
> >
>
Maxim Uvarov March 6, 2015, 3:49 p.m. UTC | #6 
On 03/06/15 16:31, Mike Holmes wrote:
>
>
> On 6 March 2015 at 04:57, Ola Liljedahl <ola.liljedahl@linaro.org 
> <mailto:ola.liljedahl@linaro.org>> wrote:
>
>     OK fixed this in v3.
>
>     If this is a style rule
>

In CONTRIBUTING it's said that we are following kernel code style.
But I did not find here requirement to define variables on top of the 
function:
https://www.kernel.org/doc/Documentation/CodingStyle

 From other point in kernel variables usually defined on not and very 
very rare
defined in the function body.

Maxim.


>     (and I approve of it, I just didn't expect
>     odp_queue_destroy() to have any return value, I thought it was succeed
>     or crash semantics like I prefer), wouldn't it be good it there was
>     some way the compiler could always warn for this?
>     GCC states: "The default is -Wunused-result" but it seems like the
>     function needs to be declared with the "warn_unused_result" attribute.
>     Is there any way to get this warning for all functions? If not,
>     perhaps we should add this function attribute to ODP functions such as
>     odp_queue_destroy()?
>
>
> I am all for adding such hints, I am going to play with it and see how 
> it pans out.
> If adding (void) suppress the warning in an application when you truly 
> don't want to look at the return code I see no harm in adding it to 
> the whole API.
>
>
>     On 5 March 2015 at 19:58, Mike Holmes <mike.holmes@linaro.org
>     <mailto:mike.holmes@linaro.org>> wrote:
>     > pasted to wrong on this v2 - meant odp_queue_destroy
>     >
>     > On 5 March 2015 at 13:57, Mike Holmes <mike.holmes@linaro.org
>     <mailto:mike.holmes@linaro.org>> wrote:
>     >>
>     >>
>     >>
>     >> On 5 March 2015 at 10:57, Bill Fischofer
>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>     >> wrote:
>     >>>
>     >>>
>     >>>
>     >>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl
>     <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>>
>     >>> wrote:
>     >>>>
>     >>>> Free queue and timeouts, destroy timeout pool before termination.
>     >>>> https://bugs.linaro.org/show_bug.cgi?id=1285
>     >>>>
>     >>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org
>     <mailto:ola.liljedahl@linaro.org>>
>     >>>
>     >>>
>     >>> Reviewed-and-tested-by: Bill Fischofer
>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>     >>>
>     >>>>
>     >>>> ---
>     >>>> (This document/code contribution attached is provided under
>     the terms of
>     >>>> agreement LES-LTM-21309)
>     >>>>
>     >>>> Removed reference to Linaro CARDS as this is internal.
>     >>>>
>     >>>>  test/validation/odp_timer.c | 9 +++++++++
>     >>>>  1 file changed, 9 insertions(+)
>     >>>>
>     >>>> diff --git a/test/validation/odp_timer.c
>     b/test/validation/odp_timer.c
>     >>>> index 88af61b..3e83367 100644
>     >>>> --- a/test/validation/odp_timer.c
>     >>>> +++ b/test/validation/odp_timer.c
>     >>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void *arg)
>     >>>>         if (ev != ODP_EVENT_INVALID)
>     >>>>                 CU_FAIL("Unexpected event received");
>     >>>>
>     >>>> +       odp_queue_destroy(queue);
>     >>>> +       for (i = 0; i < NTIMERS; i++) {
>     >>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
>     >>>> +
>     >>>> odp_timeout_free(odp_timeout_from_event(tt[i].ev));
>     >>>> +       }
>     >>>>         LOG_DBG("Thread %u: exiting\n", thr);
>     >>>>         return NULL;
>     >>>>  }
>     >>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>     >>>>         /* Destroy timer pool, all timers must have been freed */
>     >>>>         odp_timer_pool_destroy(tp);
>     >>
>     >>
>     >> Still need to set
>     >> (void)  odp_timer_pool_destroy(tp);
>     >> or
>     >> check the return code.
>     >> Else where in this file the explicit intention to ignore a
>     return code is
>     >> signaled with (void)
>     >>
>     >>
>     >>>>
>     >>>>
>     >>>> +       /* Destroy timeout pool, all timeouts must have been
>     freed */
>     >>>> +       int rc = odp_pool_destroy(tbp);
>     >>>> +       CU_ASSERT(rc == 0);
>     >>>> +
>     >>>>         CU_PASS("ODP timer test");
>     >>>>  }
>     >>>>
>     >>>> --
>     >>>> 1.9.1
>     >>>>
>     >>>>
>     >>>> _______________________________________________
>     >>>> lng-odp mailing list
>     >>>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>     >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>     >>>
>     >>>
>     >>>
>     >>> _______________________________________________
>     >>> lng-odp mailing list
>     >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>     >>> http://lists.linaro.org/mailman/listinfo/lng-odp
>     >>>
>     >>
>     >>
>     >>
>     >> --
>     >> Mike Holmes
>     >> Technical Manager - Linaro Networking Group
>     >> Linaro.org │ Open source software for ARM SoCs
>     >>
>     >>
>     >
>     >
>     >
>     > --
>     > Mike Holmes
>     > Technical Manager - Linaro Networking Group
>     > Linaro.org │ Open source software for ARM SoCs
>     >
>     >
>
>
>
>
> -- 
> Mike Holmes
> Technical Manager - Linaro Networking Group
> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM SoCs
>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Mike Holmes March 6, 2015, 3:56 p.m. UTC | #7 
On 6 March 2015 at 10:49, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

> On 03/06/15 16:31, Mike Holmes wrote:
>
>>
>>
>> On 6 March 2015 at 04:57, Ola Liljedahl <ola.liljedahl@linaro.org
>> <mailto:ola.liljedahl@linaro.org>> wrote:
>>
>>     OK fixed this in v3.
>>
>>     If this is a style rule
>>
>>
> In CONTRIBUTING it's said that we are following kernel code style.
> But I did not find here requirement to define variables on top of the
> function:
> https://www.kernel.org/doc/Documentation/CodingStyle
>
> From other point in kernel variables usually defined on not and very very
> rare
> defined in the function body.
>


Maybe we update our CONTRIBUTING,
<https://git.linaro.org/lng/odp.git/blob/HEAD:/CONTRIBUTING> but I think
our precedent is to declare variables at the top of the function, and to
#define things at the start of a source .c file if  they are needed.


> Maxim.
>
>
>      (and I approve of it, I just didn't expect
>>     odp_queue_destroy() to have any return value, I thought it was succeed
>>     or crash semantics like I prefer), wouldn't it be good it there was
>>     some way the compiler could always warn for this?
>>     GCC states: "The default is -Wunused-result" but it seems like the
>>     function needs to be declared with the "warn_unused_result" attribute.
>>     Is there any way to get this warning for all functions? If not,
>>     perhaps we should add this function attribute to ODP functions such as
>>     odp_queue_destroy()?
>>
>>
>> I am all for adding such hints, I am going to play with it and see how it
>> pans out.
>> If adding (void) suppress the warning in an application when you truly
>> don't want to look at the return code I see no harm in adding it to the
>> whole API.
>>
>>
>>     On 5 March 2015 at 19:58, Mike Holmes <mike.holmes@linaro.org
>>     <mailto:mike.holmes@linaro.org>> wrote:
>>     > pasted to wrong on this v2 - meant odp_queue_destroy
>>     >
>>     > On 5 March 2015 at 13:57, Mike Holmes <mike.holmes@linaro.org
>>     <mailto:mike.holmes@linaro.org>> wrote:
>>     >>
>>     >>
>>     >>
>>     >> On 5 March 2015 at 10:57, Bill Fischofer
>>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>     >> wrote:
>>     >>>
>>     >>>
>>     >>>
>>     >>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl
>>     <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>>
>>     >>> wrote:
>>     >>>>
>>     >>>> Free queue and timeouts, destroy timeout pool before termination.
>>     >>>> https://bugs.linaro.org/show_bug.cgi?id=1285
>>     >>>>
>>     >>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org
>>     <mailto:ola.liljedahl@linaro.org>>
>>     >>>
>>     >>>
>>     >>> Reviewed-and-tested-by: Bill Fischofer
>>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>
>>     >>>
>>     >>>>
>>     >>>> ---
>>     >>>> (This document/code contribution attached is provided under
>>     the terms of
>>     >>>> agreement LES-LTM-21309)
>>     >>>>
>>     >>>> Removed reference to Linaro CARDS as this is internal.
>>     >>>>
>>     >>>>  test/validation/odp_timer.c | 9 +++++++++
>>     >>>>  1 file changed, 9 insertions(+)
>>     >>>>
>>     >>>> diff --git a/test/validation/odp_timer.c
>>     b/test/validation/odp_timer.c
>>     >>>> index 88af61b..3e83367 100644
>>     >>>> --- a/test/validation/odp_timer.c
>>     >>>> +++ b/test/validation/odp_timer.c
>>     >>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void *arg)
>>     >>>>         if (ev != ODP_EVENT_INVALID)
>>     >>>>                 CU_FAIL("Unexpected event received");
>>     >>>>
>>     >>>> +       odp_queue_destroy(queue);
>>     >>>> +       for (i = 0; i < NTIMERS; i++) {
>>     >>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
>>     >>>> +
>>     >>>> odp_timeout_free(odp_timeout_from_event(tt[i].ev));
>>     >>>> +       }
>>     >>>>         LOG_DBG("Thread %u: exiting\n", thr);
>>     >>>>         return NULL;
>>     >>>>  }
>>     >>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>>     >>>>         /* Destroy timer pool, all timers must have been freed */
>>     >>>>         odp_timer_pool_destroy(tp);
>>     >>
>>     >>
>>     >> Still need to set
>>     >> (void)  odp_timer_pool_destroy(tp);
>>     >> or
>>     >> check the return code.
>>     >> Else where in this file the explicit intention to ignore a
>>     return code is
>>     >> signaled with (void)
>>     >>
>>     >>
>>     >>>>
>>     >>>>
>>     >>>> +       /* Destroy timeout pool, all timeouts must have been
>>     freed */
>>     >>>> +       int rc = odp_pool_destroy(tbp);
>>     >>>> +       CU_ASSERT(rc == 0);
>>     >>>> +
>>     >>>>         CU_PASS("ODP timer test");
>>     >>>>  }
>>     >>>>
>>     >>>> --
>>     >>>> 1.9.1
>>     >>>>
>>     >>>>
>>     >>>> _______________________________________________
>>     >>>> lng-odp mailing list
>>     >>>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>     >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>     >>>
>>     >>>
>>     >>>
>>     >>> _______________________________________________
>>     >>> lng-odp mailing list
>>     >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>     >>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>     >>>
>>     >>
>>     >>
>>     >>
>>     >> --
>>     >> Mike Holmes
>>     >> Technical Manager - Linaro Networking Group
>>     >> Linaro.org │ Open source software for ARM SoCs
>>     >>
>>     >>
>>     >
>>     >
>>     >
>>     > --
>>     > Mike Holmes
>>     > Technical Manager - Linaro Networking Group
>>     > Linaro.org │ Open source software for ARM SoCs
>>     >
>>     >
>>
>>
>>
>>
>> --
>> Mike Holmes
>> Technical Manager - Linaro Networking Group
>> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM
>> SoCs
>>
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
Bill Fischofer March 6, 2015, 4:36 p.m. UTC | #8 
I don't think we need a rigid policy for everything.  The kernel guidelines
are sufficient.  For variables that have limited scope, local definitions
are already common.  The main consideration here is does it make for
improved readability and maintainability. For variables used throughout a
function the declare-at-the-top rule makes sense, but for temporaries, etc.
I think more localized declaration can improve both.

On Fri, Mar 6, 2015 at 9:56 AM, Mike Holmes <mike.holmes@linaro.org> wrote:

>
>
> On 6 March 2015 at 10:49, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
>> On 03/06/15 16:31, Mike Holmes wrote:
>>
>>>
>>>
>>> On 6 March 2015 at 04:57, Ola Liljedahl <ola.liljedahl@linaro.org
>>> <mailto:ola.liljedahl@linaro.org>> wrote:
>>>
>>>     OK fixed this in v3.
>>>
>>>     If this is a style rule
>>>
>>>
>> In CONTRIBUTING it's said that we are following kernel code style.
>> But I did not find here requirement to define variables on top of the
>> function:
>> https://www.kernel.org/doc/Documentation/CodingStyle
>>
>> From other point in kernel variables usually defined on not and very very
>> rare
>> defined in the function body.
>>
>
>
> Maybe we update our CONTRIBUTING,
> <https://git.linaro.org/lng/odp.git/blob/HEAD:/CONTRIBUTING> but I think
> our precedent is to declare variables at the top of the function, and to
> #define things at the start of a source .c file if  they are needed.
>
>
>> Maxim.
>>
>>
>>      (and I approve of it, I just didn't expect
>>>     odp_queue_destroy() to have any return value, I thought it was
>>> succeed
>>>     or crash semantics like I prefer), wouldn't it be good it there was
>>>     some way the compiler could always warn for this?
>>>     GCC states: "The default is -Wunused-result" but it seems like the
>>>     function needs to be declared with the "warn_unused_result"
>>> attribute.
>>>     Is there any way to get this warning for all functions? If not,
>>>     perhaps we should add this function attribute to ODP functions such
>>> as
>>>     odp_queue_destroy()?
>>>
>>>
>>> I am all for adding such hints, I am going to play with it and see how
>>> it pans out.
>>> If adding (void) suppress the warning in an application when you truly
>>> don't want to look at the return code I see no harm in adding it to the
>>> whole API.
>>>
>>>
>>>     On 5 March 2015 at 19:58, Mike Holmes <mike.holmes@linaro.org
>>>     <mailto:mike.holmes@linaro.org>> wrote:
>>>     > pasted to wrong on this v2 - meant odp_queue_destroy
>>>     >
>>>     > On 5 March 2015 at 13:57, Mike Holmes <mike.holmes@linaro.org
>>>     <mailto:mike.holmes@linaro.org>> wrote:
>>>     >>
>>>     >>
>>>     >>
>>>     >> On 5 March 2015 at 10:57, Bill Fischofer
>>>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>>     >> wrote:
>>>     >>>
>>>     >>>
>>>     >>>
>>>     >>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl
>>>     <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>>
>>>     >>> wrote:
>>>     >>>>
>>>     >>>> Free queue and timeouts, destroy timeout pool before
>>> termination.
>>>     >>>> https://bugs.linaro.org/show_bug.cgi?id=1285
>>>     >>>>
>>>     >>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org
>>>     <mailto:ola.liljedahl@linaro.org>>
>>>     >>>
>>>     >>>
>>>     >>> Reviewed-and-tested-by: Bill Fischofer
>>>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>>
>>>     >>>
>>>     >>>>
>>>     >>>> ---
>>>     >>>> (This document/code contribution attached is provided under
>>>     the terms of
>>>     >>>> agreement LES-LTM-21309)
>>>     >>>>
>>>     >>>> Removed reference to Linaro CARDS as this is internal.
>>>     >>>>
>>>     >>>>  test/validation/odp_timer.c | 9 +++++++++
>>>     >>>>  1 file changed, 9 insertions(+)
>>>     >>>>
>>>     >>>> diff --git a/test/validation/odp_timer.c
>>>     b/test/validation/odp_timer.c
>>>     >>>> index 88af61b..3e83367 100644
>>>     >>>> --- a/test/validation/odp_timer.c
>>>     >>>> +++ b/test/validation/odp_timer.c
>>>     >>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void *arg)
>>>     >>>>         if (ev != ODP_EVENT_INVALID)
>>>     >>>>                 CU_FAIL("Unexpected event received");
>>>     >>>>
>>>     >>>> +       odp_queue_destroy(queue);
>>>     >>>> +       for (i = 0; i < NTIMERS; i++) {
>>>     >>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
>>>     >>>> +
>>>     >>>> odp_timeout_free(odp_timeout_from_event(tt[i].ev));
>>>     >>>> +       }
>>>     >>>>         LOG_DBG("Thread %u: exiting\n", thr);
>>>     >>>>         return NULL;
>>>     >>>>  }
>>>     >>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>>>     >>>>         /* Destroy timer pool, all timers must have been freed
>>> */
>>>     >>>>         odp_timer_pool_destroy(tp);
>>>     >>
>>>     >>
>>>     >> Still need to set
>>>     >> (void)  odp_timer_pool_destroy(tp);
>>>     >> or
>>>     >> check the return code.
>>>     >> Else where in this file the explicit intention to ignore a
>>>     return code is
>>>     >> signaled with (void)
>>>     >>
>>>     >>
>>>     >>>>
>>>     >>>>
>>>     >>>> +       /* Destroy timeout pool, all timeouts must have been
>>>     freed */
>>>     >>>> +       int rc = odp_pool_destroy(tbp);
>>>     >>>> +       CU_ASSERT(rc == 0);
>>>     >>>> +
>>>     >>>>         CU_PASS("ODP timer test");
>>>     >>>>  }
>>>     >>>>
>>>     >>>> --
>>>     >>>> 1.9.1
>>>     >>>>
>>>     >>>>
>>>     >>>> _______________________________________________
>>>     >>>> lng-odp mailing list
>>>     >>>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>>     >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>     >>>
>>>     >>>
>>>     >>>
>>>     >>> _______________________________________________
>>>     >>> lng-odp mailing list
>>>     >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>>     >>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>     >>>
>>>     >>
>>>     >>
>>>     >>
>>>     >> --
>>>     >> Mike Holmes
>>>     >> Technical Manager - Linaro Networking Group
>>>     >> Linaro.org │ Open source software for ARM SoCs
>>>     >>
>>>     >>
>>>     >
>>>     >
>>>     >
>>>     > --
>>>     > Mike Holmes
>>>     > Technical Manager - Linaro Networking Group
>>>     > Linaro.org │ Open source software for ARM SoCs
>>>     >
>>>     >
>>>
>>>
>>>
>>>
>>> --
>>> Mike Holmes
>>> Technical Manager - Linaro Networking Group
>>> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM
>>> SoCs
>>>
>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>
>
>
> --
> Mike Holmes
> Technical Manager - Linaro Networking Group
> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs
>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Ola Liljedahl March 6, 2015, 4:54 p.m. UTC | #9 
On 6 March 2015 at 17:36, Bill Fischofer <bill.fischofer@linaro.org> wrote:
> I don't think we need a rigid policy for everything.  The kernel guidelines
> are sufficient.  For variables that have limited scope, local definitions
> are already common.  The main consideration here is does it make for
> improved readability and maintainability. For variables used throughout a
> function the declare-at-the-top rule makes sense, but for temporaries, etc.
> I think more localized declaration can improve both.
I am all with Bill here.
Putting a use-once variable declaration at the top of the function far
away from where it is used just decreases the understanding.

In general, I think it is much better to declare variables from the
point where you use them. The compiler will complain if you attempt to
redeclare the same name. Maybe compilers weren't so good back when
Linus started to code the Linux kernel but things have since moved on
and improved. Clang is even better with warnings.

>
> On Fri, Mar 6, 2015 at 9:56 AM, Mike Holmes <mike.holmes@linaro.org> wrote:
>>
>>
>>
>> On 6 March 2015 at 10:49, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>>>
>>> On 03/06/15 16:31, Mike Holmes wrote:
>>>>
>>>>
>>>>
>>>> On 6 March 2015 at 04:57, Ola Liljedahl <ola.liljedahl@linaro.org
>>>> <mailto:ola.liljedahl@linaro.org>> wrote:
>>>>
>>>>     OK fixed this in v3.
>>>>
>>>>     If this is a style rule
>>>>
>>>
>>> In CONTRIBUTING it's said that we are following kernel code style.
>>> But I did not find here requirement to define variables on top of the
>>> function:
>>> https://www.kernel.org/doc/Documentation/CodingStyle
>>>
>>> From other point in kernel variables usually defined on not and very very
>>> rare
>>> defined in the function body.
>>
>>
>>
>> Maybe we update our CONTRIBUTING, but I think our precedent is to declare
>> variables at the top of the function, and to #define things at the start of
>> a source .c file if  they are needed.
>>
>>>
>>> Maxim.
>>>
>>>
>>>>     (and I approve of it, I just didn't expect
>>>>     odp_queue_destroy() to have any return value, I thought it was
>>>> succeed
>>>>     or crash semantics like I prefer), wouldn't it be good it there was
>>>>     some way the compiler could always warn for this?
>>>>     GCC states: "The default is -Wunused-result" but it seems like the
>>>>     function needs to be declared with the "warn_unused_result"
>>>> attribute.
>>>>     Is there any way to get this warning for all functions? If not,
>>>>     perhaps we should add this function attribute to ODP functions such
>>>> as
>>>>     odp_queue_destroy()?
>>>>
>>>>
>>>> I am all for adding such hints, I am going to play with it and see how
>>>> it pans out.
>>>> If adding (void) suppress the warning in an application when you truly
>>>> don't want to look at the return code I see no harm in adding it to the
>>>> whole API.
>>>>
>>>>
>>>>     On 5 March 2015 at 19:58, Mike Holmes <mike.holmes@linaro.org
>>>>     <mailto:mike.holmes@linaro.org>> wrote:
>>>>     > pasted to wrong on this v2 - meant odp_queue_destroy
>>>>     >
>>>>     > On 5 March 2015 at 13:57, Mike Holmes <mike.holmes@linaro.org
>>>>     <mailto:mike.holmes@linaro.org>> wrote:
>>>>     >>
>>>>     >>
>>>>     >>
>>>>     >> On 5 March 2015 at 10:57, Bill Fischofer
>>>>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>>>     >> wrote:
>>>>     >>>
>>>>     >>>
>>>>     >>>
>>>>     >>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl
>>>>     <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>>
>>>>     >>> wrote:
>>>>     >>>>
>>>>     >>>> Free queue and timeouts, destroy timeout pool before
>>>> termination.
>>>>     >>>> https://bugs.linaro.org/show_bug.cgi?id=1285
>>>>     >>>>
>>>>     >>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org
>>>>     <mailto:ola.liljedahl@linaro.org>>
>>>>     >>>
>>>>     >>>
>>>>     >>> Reviewed-and-tested-by: Bill Fischofer
>>>>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>>>
>>>>     >>>
>>>>     >>>>
>>>>     >>>> ---
>>>>     >>>> (This document/code contribution attached is provided under
>>>>     the terms of
>>>>     >>>> agreement LES-LTM-21309)
>>>>     >>>>
>>>>     >>>> Removed reference to Linaro CARDS as this is internal.
>>>>     >>>>
>>>>     >>>>  test/validation/odp_timer.c | 9 +++++++++
>>>>     >>>>  1 file changed, 9 insertions(+)
>>>>     >>>>
>>>>     >>>> diff --git a/test/validation/odp_timer.c
>>>>     b/test/validation/odp_timer.c
>>>>     >>>> index 88af61b..3e83367 100644
>>>>     >>>> --- a/test/validation/odp_timer.c
>>>>     >>>> +++ b/test/validation/odp_timer.c
>>>>     >>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void *arg)
>>>>     >>>>         if (ev != ODP_EVENT_INVALID)
>>>>     >>>>                 CU_FAIL("Unexpected event received");
>>>>     >>>>
>>>>     >>>> +       odp_queue_destroy(queue);
>>>>     >>>> +       for (i = 0; i < NTIMERS; i++) {
>>>>     >>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
>>>>     >>>> +
>>>>     >>>> odp_timeout_free(odp_timeout_from_event(tt[i].ev));
>>>>     >>>> +       }
>>>>     >>>>         LOG_DBG("Thread %u: exiting\n", thr);
>>>>     >>>>         return NULL;
>>>>     >>>>  }
>>>>     >>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>>>>     >>>>         /* Destroy timer pool, all timers must have been freed
>>>> */
>>>>     >>>>         odp_timer_pool_destroy(tp);
>>>>     >>
>>>>     >>
>>>>     >> Still need to set
>>>>     >> (void)  odp_timer_pool_destroy(tp);
>>>>     >> or
>>>>     >> check the return code.
>>>>     >> Else where in this file the explicit intention to ignore a
>>>>     return code is
>>>>     >> signaled with (void)
>>>>     >>
>>>>     >>
>>>>     >>>>
>>>>     >>>>
>>>>     >>>> +       /* Destroy timeout pool, all timeouts must have been
>>>>     freed */
>>>>     >>>> +       int rc = odp_pool_destroy(tbp);
>>>>     >>>> +       CU_ASSERT(rc == 0);
>>>>     >>>> +
>>>>     >>>>         CU_PASS("ODP timer test");
>>>>     >>>>  }
>>>>     >>>>
>>>>     >>>> --
>>>>     >>>> 1.9.1
>>>>     >>>>
>>>>     >>>>
>>>>     >>>> _______________________________________________
>>>>     >>>> lng-odp mailing list
>>>>     >>>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>>>     >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>     >>>
>>>>     >>>
>>>>     >>>
>>>>     >>> _______________________________________________
>>>>     >>> lng-odp mailing list
>>>>     >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>>>     >>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>     >>>
>>>>     >>
>>>>     >>
>>>>     >>
>>>>     >> --
>>>>     >> Mike Holmes
>>>>     >> Technical Manager - Linaro Networking Group
>>>>     >> Linaro.org │ Open source software for ARM SoCs
>>>>     >>
>>>>     >>
>>>>     >
>>>>     >
>>>>     >
>>>>     > --
>>>>     > Mike Holmes
>>>>     > Technical Manager - Linaro Networking Group
>>>>     > Linaro.org │ Open source software for ARM SoCs
>>>>     >
>>>>     >
>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Mike Holmes
>>>> Technical Manager - Linaro Networking Group
>>>> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM
>>>> SoCs
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> lng-odp mailing list
>>>> lng-odp@lists.linaro.org
>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>>
>>
>> --
>> Mike Holmes
>> Technical Manager - Linaro Networking Group
>> Linaro.org │ Open source software for ARM SoCs
>>
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
Mike Holmes March 6, 2015, 5:02 p.m. UTC | #10 
On 6 March 2015 at 11:54, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:

> On 6 March 2015 at 17:36, Bill Fischofer <bill.fischofer@linaro.org>
> wrote:
> > I don't think we need a rigid policy for everything.  The kernel
> guidelines
> > are sufficient.  For variables that have limited scope, local definitions
> > are already common.  The main consideration here is does it make for
> > improved readability and maintainability. For variables used throughout a
> > function the declare-at-the-top rule makes sense, but for temporaries,
> etc.
> > I think more localized declaration can improve both.
> I am all with Bill here.
> Putting a use-once variable declaration at the top of the function far
> away from where it is used just decreases the understanding.
>
> In general, I think it is much better to declare variables from the
> point where you use them. The compiler will complain if you attempt to
> redeclare the same name. Maybe compilers weren't so good back when
> Linus started to code the Linux kernel but things have since moved on
> and improved. Clang is even better with warnings.
>

I don't mind what the policy is, but to save people with different opinions
flagging things in patches we can take a stance, Linux is lucky there is
only one opinion.

To stave off future comments on patches in ODP what is our policy ?


>
> >
> > On Fri, Mar 6, 2015 at 9:56 AM, Mike Holmes <mike.holmes@linaro.org>
> wrote:
> >>
> >>
> >>
> >> On 6 March 2015 at 10:49, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> >>>
> >>> On 03/06/15 16:31, Mike Holmes wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 6 March 2015 at 04:57, Ola Liljedahl <ola.liljedahl@linaro.org
> >>>> <mailto:ola.liljedahl@linaro.org>> wrote:
> >>>>
> >>>>     OK fixed this in v3.
> >>>>
> >>>>     If this is a style rule
> >>>>
> >>>
> >>> In CONTRIBUTING it's said that we are following kernel code style.
> >>> But I did not find here requirement to define variables on top of the
> >>> function:
> >>> https://www.kernel.org/doc/Documentation/CodingStyle
> >>>
> >>> From other point in kernel variables usually defined on not and very
> very
> >>> rare
> >>> defined in the function body.
> >>
> >>
> >>
> >> Maybe we update our CONTRIBUTING, but I think our precedent is to
> declare
> >> variables at the top of the function, and to #define things at the
> start of
> >> a source .c file if  they are needed.
> >>
> >>>
> >>> Maxim.
> >>>
> >>>
> >>>>     (and I approve of it, I just didn't expect
> >>>>     odp_queue_destroy() to have any return value, I thought it was
> >>>> succeed
> >>>>     or crash semantics like I prefer), wouldn't it be good it there
> was
> >>>>     some way the compiler could always warn for this?
> >>>>     GCC states: "The default is -Wunused-result" but it seems like the
> >>>>     function needs to be declared with the "warn_unused_result"
> >>>> attribute.
> >>>>     Is there any way to get this warning for all functions? If not,
> >>>>     perhaps we should add this function attribute to ODP functions
> such
> >>>> as
> >>>>     odp_queue_destroy()?
> >>>>
> >>>>
> >>>> I am all for adding such hints, I am going to play with it and see how
> >>>> it pans out.
> >>>> If adding (void) suppress the warning in an application when you truly
> >>>> don't want to look at the return code I see no harm in adding it to
> the
> >>>> whole API.
> >>>>
> >>>>
> >>>>     On 5 March 2015 at 19:58, Mike Holmes <mike.holmes@linaro.org
> >>>>     <mailto:mike.holmes@linaro.org>> wrote:
> >>>>     > pasted to wrong on this v2 - meant odp_queue_destroy
> >>>>     >
> >>>>     > On 5 March 2015 at 13:57, Mike Holmes <mike.holmes@linaro.org
> >>>>     <mailto:mike.holmes@linaro.org>> wrote:
> >>>>     >>
> >>>>     >>
> >>>>     >>
> >>>>     >> On 5 March 2015 at 10:57, Bill Fischofer
> >>>>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
> >>>>     >> wrote:
> >>>>     >>>
> >>>>     >>>
> >>>>     >>>
> >>>>     >>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl
> >>>>     <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>>
> >>>>     >>> wrote:
> >>>>     >>>>
> >>>>     >>>> Free queue and timeouts, destroy timeout pool before
> >>>> termination.
> >>>>     >>>> https://bugs.linaro.org/show_bug.cgi?id=1285
> >>>>     >>>>
> >>>>     >>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org
> >>>>     <mailto:ola.liljedahl@linaro.org>>
> >>>>     >>>
> >>>>     >>>
> >>>>     >>> Reviewed-and-tested-by: Bill Fischofer
> >>>>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
> >>>>
> >>>>     >>>
> >>>>     >>>>
> >>>>     >>>> ---
> >>>>     >>>> (This document/code contribution attached is provided under
> >>>>     the terms of
> >>>>     >>>> agreement LES-LTM-21309)
> >>>>     >>>>
> >>>>     >>>> Removed reference to Linaro CARDS as this is internal.
> >>>>     >>>>
> >>>>     >>>>  test/validation/odp_timer.c | 9 +++++++++
> >>>>     >>>>  1 file changed, 9 insertions(+)
> >>>>     >>>>
> >>>>     >>>> diff --git a/test/validation/odp_timer.c
> >>>>     b/test/validation/odp_timer.c
> >>>>     >>>> index 88af61b..3e83367 100644
> >>>>     >>>> --- a/test/validation/odp_timer.c
> >>>>     >>>> +++ b/test/validation/odp_timer.c
> >>>>     >>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void
> *arg)
> >>>>     >>>>         if (ev != ODP_EVENT_INVALID)
> >>>>     >>>>                 CU_FAIL("Unexpected event received");
> >>>>     >>>>
> >>>>     >>>> +       odp_queue_destroy(queue);
> >>>>     >>>> +       for (i = 0; i < NTIMERS; i++) {
> >>>>     >>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
> >>>>     >>>> +
> >>>>     >>>> odp_timeout_free(odp_timeout_from_event(tt[i].ev));
> >>>>     >>>> +       }
> >>>>     >>>>         LOG_DBG("Thread %u: exiting\n", thr);
> >>>>     >>>>         return NULL;
> >>>>     >>>>  }
> >>>>     >>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
> >>>>     >>>>         /* Destroy timer pool, all timers must have been
> freed
> >>>> */
> >>>>     >>>>         odp_timer_pool_destroy(tp);
> >>>>     >>
> >>>>     >>
> >>>>     >> Still need to set
> >>>>     >> (void)  odp_timer_pool_destroy(tp);
> >>>>     >> or
> >>>>     >> check the return code.
> >>>>     >> Else where in this file the explicit intention to ignore a
> >>>>     return code is
> >>>>     >> signaled with (void)
> >>>>     >>
> >>>>     >>
> >>>>     >>>>
> >>>>     >>>>
> >>>>     >>>> +       /* Destroy timeout pool, all timeouts must have been
> >>>>     freed */
> >>>>     >>>> +       int rc = odp_pool_destroy(tbp);
> >>>>     >>>> +       CU_ASSERT(rc == 0);
> >>>>     >>>> +
> >>>>     >>>>         CU_PASS("ODP timer test");
> >>>>     >>>>  }
> >>>>     >>>>
> >>>>     >>>> --
> >>>>     >>>> 1.9.1
> >>>>     >>>>
> >>>>     >>>>
> >>>>     >>>> _______________________________________________
> >>>>     >>>> lng-odp mailing list
> >>>>     >>>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
> >>>>     >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
> >>>>     >>>
> >>>>     >>>
> >>>>     >>>
> >>>>     >>> _______________________________________________
> >>>>     >>> lng-odp mailing list
> >>>>     >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
> >>>>     >>> http://lists.linaro.org/mailman/listinfo/lng-odp
> >>>>     >>>
> >>>>     >>
> >>>>     >>
> >>>>     >>
> >>>>     >> --
> >>>>     >> Mike Holmes
> >>>>     >> Technical Manager - Linaro Networking Group
> >>>>     >> Linaro.org │ Open source software for ARM SoCs
> >>>>     >>
> >>>>     >>
> >>>>     >
> >>>>     >
> >>>>     >
> >>>>     > --
> >>>>     > Mike Holmes
> >>>>     > Technical Manager - Linaro Networking Group
> >>>>     > Linaro.org │ Open source software for ARM SoCs
> >>>>     >
> >>>>     >
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> --
> >>>> Mike Holmes
> >>>> Technical Manager - Linaro Networking Group
> >>>> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM
> >>>> SoCs
> >>>>
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> lng-odp mailing list
> >>>> lng-odp@lists.linaro.org
> >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
> >>>
> >>>
> >>>
> >>> _______________________________________________
> >>> lng-odp mailing list
> >>> lng-odp@lists.linaro.org
> >>> http://lists.linaro.org/mailman/listinfo/lng-odp
> >>
> >>
> >>
> >>
> >> --
> >> Mike Holmes
> >> Technical Manager - Linaro Networking Group
> >> Linaro.org │ Open source software for ARM SoCs
> >>
> >>
> >>
> >> _______________________________________________
> >> lng-odp mailing list
> >> lng-odp@lists.linaro.org
> >> http://lists.linaro.org/mailman/listinfo/lng-odp
> >>
> >
> >
> > _______________________________________________
> > lng-odp mailing list
> > lng-odp@lists.linaro.org
> > http://lists.linaro.org/mailman/listinfo/lng-odp
> >
>
Bill Fischofer March 6, 2015, 5:03 p.m. UTC | #11 
I think our policy is: Use good judgement in everything.  :)

On Fri, Mar 6, 2015 at 11:02 AM, Mike Holmes <mike.holmes@linaro.org> wrote:

>
>
> On 6 March 2015 at 11:54, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:
>
>> On 6 March 2015 at 17:36, Bill Fischofer <bill.fischofer@linaro.org>
>> wrote:
>> > I don't think we need a rigid policy for everything.  The kernel
>> guidelines
>> > are sufficient.  For variables that have limited scope, local
>> definitions
>> > are already common.  The main consideration here is does it make for
>> > improved readability and maintainability. For variables used throughout
>> a
>> > function the declare-at-the-top rule makes sense, but for temporaries,
>> etc.
>> > I think more localized declaration can improve both.
>> I am all with Bill here.
>> Putting a use-once variable declaration at the top of the function far
>> away from where it is used just decreases the understanding.
>>
>> In general, I think it is much better to declare variables from the
>> point where you use them. The compiler will complain if you attempt to
>> redeclare the same name. Maybe compilers weren't so good back when
>> Linus started to code the Linux kernel but things have since moved on
>> and improved. Clang is even better with warnings.
>>
>
> I don't mind what the policy is, but to save people with different
> opinions flagging things in patches we can take a stance, Linux is lucky
> there is only one opinion.
>
> To stave off future comments on patches in ODP what is our policy ?
>
>
>>
>> >
>> > On Fri, Mar 6, 2015 at 9:56 AM, Mike Holmes <mike.holmes@linaro.org>
>> wrote:
>> >>
>> >>
>> >>
>> >> On 6 March 2015 at 10:49, Maxim Uvarov <maxim.uvarov@linaro.org>
>> wrote:
>> >>>
>> >>> On 03/06/15 16:31, Mike Holmes wrote:
>> >>>>
>> >>>>
>> >>>>
>> >>>> On 6 March 2015 at 04:57, Ola Liljedahl <ola.liljedahl@linaro.org
>> >>>> <mailto:ola.liljedahl@linaro.org>> wrote:
>> >>>>
>> >>>>     OK fixed this in v3.
>> >>>>
>> >>>>     If this is a style rule
>> >>>>
>> >>>
>> >>> In CONTRIBUTING it's said that we are following kernel code style.
>> >>> But I did not find here requirement to define variables on top of the
>> >>> function:
>> >>> https://www.kernel.org/doc/Documentation/CodingStyle
>> >>>
>> >>> From other point in kernel variables usually defined on not and very
>> very
>> >>> rare
>> >>> defined in the function body.
>> >>
>> >>
>> >>
>> >> Maybe we update our CONTRIBUTING, but I think our precedent is to
>> declare
>> >> variables at the top of the function, and to #define things at the
>> start of
>> >> a source .c file if  they are needed.
>> >>
>> >>>
>> >>> Maxim.
>> >>>
>> >>>
>> >>>>     (and I approve of it, I just didn't expect
>> >>>>     odp_queue_destroy() to have any return value, I thought it was
>> >>>> succeed
>> >>>>     or crash semantics like I prefer), wouldn't it be good it there
>> was
>> >>>>     some way the compiler could always warn for this?
>> >>>>     GCC states: "The default is -Wunused-result" but it seems like
>> the
>> >>>>     function needs to be declared with the "warn_unused_result"
>> >>>> attribute.
>> >>>>     Is there any way to get this warning for all functions? If not,
>> >>>>     perhaps we should add this function attribute to ODP functions
>> such
>> >>>> as
>> >>>>     odp_queue_destroy()?
>> >>>>
>> >>>>
>> >>>> I am all for adding such hints, I am going to play with it and see
>> how
>> >>>> it pans out.
>> >>>> If adding (void) suppress the warning in an application when you
>> truly
>> >>>> don't want to look at the return code I see no harm in adding it to
>> the
>> >>>> whole API.
>> >>>>
>> >>>>
>> >>>>     On 5 March 2015 at 19:58, Mike Holmes <mike.holmes@linaro.org
>> >>>>     <mailto:mike.holmes@linaro.org>> wrote:
>> >>>>     > pasted to wrong on this v2 - meant odp_queue_destroy
>> >>>>     >
>> >>>>     > On 5 March 2015 at 13:57, Mike Holmes <mike.holmes@linaro.org
>> >>>>     <mailto:mike.holmes@linaro.org>> wrote:
>> >>>>     >>
>> >>>>     >>
>> >>>>     >>
>> >>>>     >> On 5 March 2015 at 10:57, Bill Fischofer
>> >>>>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>> >>>>     >> wrote:
>> >>>>     >>>
>> >>>>     >>>
>> >>>>     >>>
>> >>>>     >>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl
>> >>>>     <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>>
>> >>>>     >>> wrote:
>> >>>>     >>>>
>> >>>>     >>>> Free queue and timeouts, destroy timeout pool before
>> >>>> termination.
>> >>>>     >>>> https://bugs.linaro.org/show_bug.cgi?id=1285
>> >>>>     >>>>
>> >>>>     >>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org
>> >>>>     <mailto:ola.liljedahl@linaro.org>>
>> >>>>     >>>
>> >>>>     >>>
>> >>>>     >>> Reviewed-and-tested-by: Bill Fischofer
>> >>>>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>> >>>>
>> >>>>     >>>
>> >>>>     >>>>
>> >>>>     >>>> ---
>> >>>>     >>>> (This document/code contribution attached is provided under
>> >>>>     the terms of
>> >>>>     >>>> agreement LES-LTM-21309)
>> >>>>     >>>>
>> >>>>     >>>> Removed reference to Linaro CARDS as this is internal.
>> >>>>     >>>>
>> >>>>     >>>>  test/validation/odp_timer.c | 9 +++++++++
>> >>>>     >>>>  1 file changed, 9 insertions(+)
>> >>>>     >>>>
>> >>>>     >>>> diff --git a/test/validation/odp_timer.c
>> >>>>     b/test/validation/odp_timer.c
>> >>>>     >>>> index 88af61b..3e83367 100644
>> >>>>     >>>> --- a/test/validation/odp_timer.c
>> >>>>     >>>> +++ b/test/validation/odp_timer.c
>> >>>>     >>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void
>> *arg)
>> >>>>     >>>>         if (ev != ODP_EVENT_INVALID)
>> >>>>     >>>>                 CU_FAIL("Unexpected event received");
>> >>>>     >>>>
>> >>>>     >>>> +       odp_queue_destroy(queue);
>> >>>>     >>>> +       for (i = 0; i < NTIMERS; i++) {
>> >>>>     >>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
>> >>>>     >>>> +
>> >>>>     >>>> odp_timeout_free(odp_timeout_from_event(tt[i].ev));
>> >>>>     >>>> +       }
>> >>>>     >>>>         LOG_DBG("Thread %u: exiting\n", thr);
>> >>>>     >>>>         return NULL;
>> >>>>     >>>>  }
>> >>>>     >>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>> >>>>     >>>>         /* Destroy timer pool, all timers must have been
>> freed
>> >>>> */
>> >>>>     >>>>         odp_timer_pool_destroy(tp);
>> >>>>     >>
>> >>>>     >>
>> >>>>     >> Still need to set
>> >>>>     >> (void)  odp_timer_pool_destroy(tp);
>> >>>>     >> or
>> >>>>     >> check the return code.
>> >>>>     >> Else where in this file the explicit intention to ignore a
>> >>>>     return code is
>> >>>>     >> signaled with (void)
>> >>>>     >>
>> >>>>     >>
>> >>>>     >>>>
>> >>>>     >>>>
>> >>>>     >>>> +       /* Destroy timeout pool, all timeouts must have been
>> >>>>     freed */
>> >>>>     >>>> +       int rc = odp_pool_destroy(tbp);
>> >>>>     >>>> +       CU_ASSERT(rc == 0);
>> >>>>     >>>> +
>> >>>>     >>>>         CU_PASS("ODP timer test");
>> >>>>     >>>>  }
>> >>>>     >>>>
>> >>>>     >>>> --
>> >>>>     >>>> 1.9.1
>> >>>>     >>>>
>> >>>>     >>>>
>> >>>>     >>>> _______________________________________________
>> >>>>     >>>> lng-odp mailing list
>> >>>>     >>>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>> >>>>     >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>> >>>>     >>>
>> >>>>     >>>
>> >>>>     >>>
>> >>>>     >>> _______________________________________________
>> >>>>     >>> lng-odp mailing list
>> >>>>     >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>> >>>>     >>> http://lists.linaro.org/mailman/listinfo/lng-odp
>> >>>>     >>>
>> >>>>     >>
>> >>>>     >>
>> >>>>     >>
>> >>>>     >> --
>> >>>>     >> Mike Holmes
>> >>>>     >> Technical Manager - Linaro Networking Group
>> >>>>     >> Linaro.org │ Open source software for ARM SoCs
>> >>>>     >>
>> >>>>     >>
>> >>>>     >
>> >>>>     >
>> >>>>     >
>> >>>>     > --
>> >>>>     > Mike Holmes
>> >>>>     > Technical Manager - Linaro Networking Group
>> >>>>     > Linaro.org │ Open source software for ARM SoCs
>> >>>>     >
>> >>>>     >
>> >>>>
>> >>>>
>> >>>>
>> >>>>
>> >>>> --
>> >>>> Mike Holmes
>> >>>> Technical Manager - Linaro Networking Group
>> >>>> Linaro.org <http://www.linaro.org/>***│ *Open source software for
>> ARM
>> >>>> SoCs
>> >>>>
>> >>>>
>> >>>>
>> >>>> _______________________________________________
>> >>>> lng-odp mailing list
>> >>>> lng-odp@lists.linaro.org
>> >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>> >>>
>> >>>
>> >>>
>> >>> _______________________________________________
>> >>> lng-odp mailing list
>> >>> lng-odp@lists.linaro.org
>> >>> http://lists.linaro.org/mailman/listinfo/lng-odp
>> >>
>> >>
>> >>
>> >>
>> >> --
>> >> Mike Holmes
>> >> Technical Manager - Linaro Networking Group
>> >> Linaro.org │ Open source software for ARM SoCs
>> >>
>> >>
>> >>
>> >> _______________________________________________
>> >> lng-odp mailing list
>> >> lng-odp@lists.linaro.org
>> >> http://lists.linaro.org/mailman/listinfo/lng-odp
>> >>
>> >
>> >
>> > _______________________________________________
>> > lng-odp mailing list
>> > lng-odp@lists.linaro.org
>> > http://lists.linaro.org/mailman/listinfo/lng-odp
>> >
>>
>
>
>
> --
> Mike Holmes
> Technical Manager - Linaro Networking Group
> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs
>
>
>
Mike Holmes March 6, 2015, 5:06 p.m. UTC | #12 
On 6 March 2015 at 12:03, Bill Fischofer <bill.fischofer@linaro.org> wrote:

> I think our policy is: Use good judgement in everything.  :)
>

I can support that, so as maintainer then it is what Maxim thinks is
correct by his good judgment, whether a submitter likes it or not.
Any other mechanism will just waste bandwidth arguing with Maxim and it
wont get committed.



>
> On Fri, Mar 6, 2015 at 11:02 AM, Mike Holmes <mike.holmes@linaro.org>
> wrote:
>
>>
>>
>> On 6 March 2015 at 11:54, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:
>>
>>> On 6 March 2015 at 17:36, Bill Fischofer <bill.fischofer@linaro.org>
>>> wrote:
>>> > I don't think we need a rigid policy for everything.  The kernel
>>> guidelines
>>> > are sufficient.  For variables that have limited scope, local
>>> definitions
>>> > are already common.  The main consideration here is does it make for
>>> > improved readability and maintainability. For variables used
>>> throughout a
>>> > function the declare-at-the-top rule makes sense, but for temporaries,
>>> etc.
>>> > I think more localized declaration can improve both.
>>> I am all with Bill here.
>>> Putting a use-once variable declaration at the top of the function far
>>> away from where it is used just decreases the understanding.
>>>
>>> In general, I think it is much better to declare variables from the
>>> point where you use them. The compiler will complain if you attempt to
>>> redeclare the same name. Maybe compilers weren't so good back when
>>> Linus started to code the Linux kernel but things have since moved on
>>> and improved. Clang is even better with warnings.
>>>
>>
>> I don't mind what the policy is, but to save people with different
>> opinions flagging things in patches we can take a stance, Linux is lucky
>> there is only one opinion.
>>
>> To stave off future comments on patches in ODP what is our policy ?
>>
>>
>>>
>>> >
>>> > On Fri, Mar 6, 2015 at 9:56 AM, Mike Holmes <mike.holmes@linaro.org>
>>> wrote:
>>> >>
>>> >>
>>> >>
>>> >> On 6 March 2015 at 10:49, Maxim Uvarov <maxim.uvarov@linaro.org>
>>> wrote:
>>> >>>
>>> >>> On 03/06/15 16:31, Mike Holmes wrote:
>>> >>>>
>>> >>>>
>>> >>>>
>>> >>>> On 6 March 2015 at 04:57, Ola Liljedahl <ola.liljedahl@linaro.org
>>> >>>> <mailto:ola.liljedahl@linaro.org>> wrote:
>>> >>>>
>>> >>>>     OK fixed this in v3.
>>> >>>>
>>> >>>>     If this is a style rule
>>> >>>>
>>> >>>
>>> >>> In CONTRIBUTING it's said that we are following kernel code style.
>>> >>> But I did not find here requirement to define variables on top of the
>>> >>> function:
>>> >>> https://www.kernel.org/doc/Documentation/CodingStyle
>>> >>>
>>> >>> From other point in kernel variables usually defined on not and very
>>> very
>>> >>> rare
>>> >>> defined in the function body.
>>> >>
>>> >>
>>> >>
>>> >> Maybe we update our CONTRIBUTING, but I think our precedent is to
>>> declare
>>> >> variables at the top of the function, and to #define things at the
>>> start of
>>> >> a source .c file if  they are needed.
>>> >>
>>> >>>
>>> >>> Maxim.
>>> >>>
>>> >>>
>>> >>>>     (and I approve of it, I just didn't expect
>>> >>>>     odp_queue_destroy() to have any return value, I thought it was
>>> >>>> succeed
>>> >>>>     or crash semantics like I prefer), wouldn't it be good it there
>>> was
>>> >>>>     some way the compiler could always warn for this?
>>> >>>>     GCC states: "The default is -Wunused-result" but it seems like
>>> the
>>> >>>>     function needs to be declared with the "warn_unused_result"
>>> >>>> attribute.
>>> >>>>     Is there any way to get this warning for all functions? If not,
>>> >>>>     perhaps we should add this function attribute to ODP functions
>>> such
>>> >>>> as
>>> >>>>     odp_queue_destroy()?
>>> >>>>
>>> >>>>
>>> >>>> I am all for adding such hints, I am going to play with it and see
>>> how
>>> >>>> it pans out.
>>> >>>> If adding (void) suppress the warning in an application when you
>>> truly
>>> >>>> don't want to look at the return code I see no harm in adding it to
>>> the
>>> >>>> whole API.
>>> >>>>
>>> >>>>
>>> >>>>     On 5 March 2015 at 19:58, Mike Holmes <mike.holmes@linaro.org
>>> >>>>     <mailto:mike.holmes@linaro.org>> wrote:
>>> >>>>     > pasted to wrong on this v2 - meant odp_queue_destroy
>>> >>>>     >
>>> >>>>     > On 5 March 2015 at 13:57, Mike Holmes <mike.holmes@linaro.org
>>> >>>>     <mailto:mike.holmes@linaro.org>> wrote:
>>> >>>>     >>
>>> >>>>     >>
>>> >>>>     >>
>>> >>>>     >> On 5 March 2015 at 10:57, Bill Fischofer
>>> >>>>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>> >>>>     >> wrote:
>>> >>>>     >>>
>>> >>>>     >>>
>>> >>>>     >>>
>>> >>>>     >>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl
>>> >>>>     <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>>
>>> >>>>     >>> wrote:
>>> >>>>     >>>>
>>> >>>>     >>>> Free queue and timeouts, destroy timeout pool before
>>> >>>> termination.
>>> >>>>     >>>> https://bugs.linaro.org/show_bug.cgi?id=1285
>>> >>>>     >>>>
>>> >>>>     >>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org
>>> >>>>     <mailto:ola.liljedahl@linaro.org>>
>>> >>>>     >>>
>>> >>>>     >>>
>>> >>>>     >>> Reviewed-and-tested-by: Bill Fischofer
>>> >>>>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>> >>>>
>>> >>>>     >>>
>>> >>>>     >>>>
>>> >>>>     >>>> ---
>>> >>>>     >>>> (This document/code contribution attached is provided under
>>> >>>>     the terms of
>>> >>>>     >>>> agreement LES-LTM-21309)
>>> >>>>     >>>>
>>> >>>>     >>>> Removed reference to Linaro CARDS as this is internal.
>>> >>>>     >>>>
>>> >>>>     >>>>  test/validation/odp_timer.c | 9 +++++++++
>>> >>>>     >>>>  1 file changed, 9 insertions(+)
>>> >>>>     >>>>
>>> >>>>     >>>> diff --git a/test/validation/odp_timer.c
>>> >>>>     b/test/validation/odp_timer.c
>>> >>>>     >>>> index 88af61b..3e83367 100644
>>> >>>>     >>>> --- a/test/validation/odp_timer.c
>>> >>>>     >>>> +++ b/test/validation/odp_timer.c
>>> >>>>     >>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void
>>> *arg)
>>> >>>>     >>>>         if (ev != ODP_EVENT_INVALID)
>>> >>>>     >>>>                 CU_FAIL("Unexpected event received");
>>> >>>>     >>>>
>>> >>>>     >>>> +       odp_queue_destroy(queue);
>>> >>>>     >>>> +       for (i = 0; i < NTIMERS; i++) {
>>> >>>>     >>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
>>> >>>>     >>>> +
>>> >>>>     >>>> odp_timeout_free(odp_timeout_from_event(tt[i].ev));
>>> >>>>     >>>> +       }
>>> >>>>     >>>>         LOG_DBG("Thread %u: exiting\n", thr);
>>> >>>>     >>>>         return NULL;
>>> >>>>     >>>>  }
>>> >>>>     >>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>>> >>>>     >>>>         /* Destroy timer pool, all timers must have been
>>> freed
>>> >>>> */
>>> >>>>     >>>>         odp_timer_pool_destroy(tp);
>>> >>>>     >>
>>> >>>>     >>
>>> >>>>     >> Still need to set
>>> >>>>     >> (void)  odp_timer_pool_destroy(tp);
>>> >>>>     >> or
>>> >>>>     >> check the return code.
>>> >>>>     >> Else where in this file the explicit intention to ignore a
>>> >>>>     return code is
>>> >>>>     >> signaled with (void)
>>> >>>>     >>
>>> >>>>     >>
>>> >>>>     >>>>
>>> >>>>     >>>>
>>> >>>>     >>>> +       /* Destroy timeout pool, all timeouts must have
>>> been
>>> >>>>     freed */
>>> >>>>     >>>> +       int rc = odp_pool_destroy(tbp);
>>> >>>>     >>>> +       CU_ASSERT(rc == 0);
>>> >>>>     >>>> +
>>> >>>>     >>>>         CU_PASS("ODP timer test");
>>> >>>>     >>>>  }
>>> >>>>     >>>>
>>> >>>>     >>>> --
>>> >>>>     >>>> 1.9.1
>>> >>>>     >>>>
>>> >>>>     >>>>
>>> >>>>     >>>> _______________________________________________
>>> >>>>     >>>> lng-odp mailing list
>>> >>>>     >>>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>> >>>>     >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>> >>>>     >>>
>>> >>>>     >>>
>>> >>>>     >>>
>>> >>>>     >>> _______________________________________________
>>> >>>>     >>> lng-odp mailing list
>>> >>>>     >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>> >>>>     >>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>> >>>>     >>>
>>> >>>>     >>
>>> >>>>     >>
>>> >>>>     >>
>>> >>>>     >> --
>>> >>>>     >> Mike Holmes
>>> >>>>     >> Technical Manager - Linaro Networking Group
>>> >>>>     >> Linaro.org │ Open source software for ARM SoCs
>>> >>>>     >>
>>> >>>>     >>
>>> >>>>     >
>>> >>>>     >
>>> >>>>     >
>>> >>>>     > --
>>> >>>>     > Mike Holmes
>>> >>>>     > Technical Manager - Linaro Networking Group
>>> >>>>     > Linaro.org │ Open source software for ARM SoCs
>>> >>>>     >
>>> >>>>     >
>>> >>>>
>>> >>>>
>>> >>>>
>>> >>>>
>>> >>>> --
>>> >>>> Mike Holmes
>>> >>>> Technical Manager - Linaro Networking Group
>>> >>>> Linaro.org <http://www.linaro.org/>***│ *Open source software for
>>> ARM
>>> >>>> SoCs
>>> >>>>
>>> >>>>
>>> >>>>
>>> >>>> _______________________________________________
>>> >>>> lng-odp mailing list
>>> >>>> lng-odp@lists.linaro.org
>>> >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>> >>>
>>> >>>
>>> >>>
>>> >>> _______________________________________________
>>> >>> lng-odp mailing list
>>> >>> lng-odp@lists.linaro.org
>>> >>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>> >>
>>> >>
>>> >>
>>> >>
>>> >> --
>>> >> Mike Holmes
>>> >> Technical Manager - Linaro Networking Group
>>> >> Linaro.org │ Open source software for ARM SoCs
>>> >>
>>> >>
>>> >>
>>> >> _______________________________________________
>>> >> lng-odp mailing list
>>> >> lng-odp@lists.linaro.org
>>> >> http://lists.linaro.org/mailman/listinfo/lng-odp
>>> >>
>>> >
>>> >
>>> > _______________________________________________
>>> > lng-odp mailing list
>>> > lng-odp@lists.linaro.org
>>> > http://lists.linaro.org/mailman/listinfo/lng-odp
>>> >
>>>
>>
>>
>>
>> --
>> Mike Holmes
>> Technical Manager - Linaro Networking Group
>> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs
>>
>>
>>
>
Maxim Uvarov March 6, 2015, 5:14 p.m. UTC | #13 
On 03/06/15 19:54, Ola Liljedahl wrote:
> On 6 March 2015 at 17:36, Bill Fischofer <bill.fischofer@linaro.org> wrote:
>> I don't think we need a rigid policy for everything.  The kernel guidelines
>> are sufficient.  For variables that have limited scope, local definitions
>> are already common.  The main consideration here is does it make for
>> improved readability and maintainability. For variables used throughout a
>> function the declare-at-the-top rule makes sense, but for temporaries, etc.
>> I think more localized declaration can improve both.
> I am all with Bill here.
> Putting a use-once variable declaration at the top of the function far
> away from where it is used just decreases the understanding.
>
> In general, I think it is much better to declare variables from the
> point where you use them. The compiler will complain if you attempt to
> redeclare the same name. Maybe compilers weren't so good back when
> Linus started to code the Linux kernel but things have since moved on
> and improved. Clang is even better with warnings.

I think by that they also wanted to say if you need to declare variable 
in the middle of
the function then probably you need 2 functions.

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle
<snip>
     Chapter 6: Functions

Functions should be short and sweet, and do just one thing.  They should
fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
as we all know), and do one thing and do that well.
,,,,
Another measure of the function is the number of local variables. They
shouldn't exceed 5-10, or you're doing something wrong.  Re-think the
function, and split it into smaller pieces.  A human brain can
generally easily keep track of about 7 different things, anything more
and it gets confused.  You know you're brilliant, but maybe you'd like
to understand what you did 2 weeks from now.
</snip>

I used to see variables on top. And not things like "for (int i = 0;.... "
I.e. more ANSI C and not C99.

But might be there is some performance benefits, like better stack 
optimization
if all variables declared on top.

Maxim.

>> On Fri, Mar 6, 2015 at 9:56 AM, Mike Holmes <mike.holmes@linaro.org> wrote:
>>>
>>>
>>> On 6 March 2015 at 10:49, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>>>> On 03/06/15 16:31, Mike Holmes wrote:
>>>>>
>>>>>
>>>>> On 6 March 2015 at 04:57, Ola Liljedahl <ola.liljedahl@linaro.org
>>>>> <mailto:ola.liljedahl@linaro.org>> wrote:
>>>>>
>>>>>      OK fixed this in v3.
>>>>>
>>>>>      If this is a style rule
>>>>>
>>>> In CONTRIBUTING it's said that we are following kernel code style.
>>>> But I did not find here requirement to define variables on top of the
>>>> function:
>>>> https://www.kernel.org/doc/Documentation/CodingStyle
>>>>
>>>>  From other point in kernel variables usually defined on not and very very
>>>> rare
>>>> defined in the function body.
>>>
>>>
>>> Maybe we update our CONTRIBUTING, but I think our precedent is to declare
>>> variables at the top of the function, and to #define things at the start of
>>> a source .c file if  they are needed.
>>>
>>>> Maxim.
>>>>
>>>>
>>>>>      (and I approve of it, I just didn't expect
>>>>>      odp_queue_destroy() to have any return value, I thought it was
>>>>> succeed
>>>>>      or crash semantics like I prefer), wouldn't it be good it there was
>>>>>      some way the compiler could always warn for this?
>>>>>      GCC states: "The default is -Wunused-result" but it seems like the
>>>>>      function needs to be declared with the "warn_unused_result"
>>>>> attribute.
>>>>>      Is there any way to get this warning for all functions? If not,
>>>>>      perhaps we should add this function attribute to ODP functions such
>>>>> as
>>>>>      odp_queue_destroy()?
>>>>>
>>>>>
>>>>> I am all for adding such hints, I am going to play with it and see how
>>>>> it pans out.
>>>>> If adding (void) suppress the warning in an application when you truly
>>>>> don't want to look at the return code I see no harm in adding it to the
>>>>> whole API.
>>>>>
>>>>>
>>>>>      On 5 March 2015 at 19:58, Mike Holmes <mike.holmes@linaro.org
>>>>>      <mailto:mike.holmes@linaro.org>> wrote:
>>>>>      > pasted to wrong on this v2 - meant odp_queue_destroy
>>>>>      >
>>>>>      > On 5 March 2015 at 13:57, Mike Holmes <mike.holmes@linaro.org
>>>>>      <mailto:mike.holmes@linaro.org>> wrote:
>>>>>      >>
>>>>>      >>
>>>>>      >>
>>>>>      >> On 5 March 2015 at 10:57, Bill Fischofer
>>>>>      <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>>>>      >> wrote:
>>>>>      >>>
>>>>>      >>>
>>>>>      >>>
>>>>>      >>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl
>>>>>      <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>>
>>>>>      >>> wrote:
>>>>>      >>>>
>>>>>      >>>> Free queue and timeouts, destroy timeout pool before
>>>>> termination.
>>>>>      >>>> https://bugs.linaro.org/show_bug.cgi?id=1285
>>>>>      >>>>
>>>>>      >>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org
>>>>>      <mailto:ola.liljedahl@linaro.org>>
>>>>>      >>>
>>>>>      >>>
>>>>>      >>> Reviewed-and-tested-by: Bill Fischofer
>>>>>      <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>>>>
>>>>>      >>>
>>>>>      >>>>
>>>>>      >>>> ---
>>>>>      >>>> (This document/code contribution attached is provided under
>>>>>      the terms of
>>>>>      >>>> agreement LES-LTM-21309)
>>>>>      >>>>
>>>>>      >>>> Removed reference to Linaro CARDS as this is internal.
>>>>>      >>>>
>>>>>      >>>>  test/validation/odp_timer.c | 9 +++++++++
>>>>>      >>>>  1 file changed, 9 insertions(+)
>>>>>      >>>>
>>>>>      >>>> diff --git a/test/validation/odp_timer.c
>>>>>      b/test/validation/odp_timer.c
>>>>>      >>>> index 88af61b..3e83367 100644
>>>>>      >>>> --- a/test/validation/odp_timer.c
>>>>>      >>>> +++ b/test/validation/odp_timer.c
>>>>>      >>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void *arg)
>>>>>      >>>>         if (ev != ODP_EVENT_INVALID)
>>>>>      >>>>                 CU_FAIL("Unexpected event received");
>>>>>      >>>>
>>>>>      >>>> +       odp_queue_destroy(queue);
>>>>>      >>>> +       for (i = 0; i < NTIMERS; i++) {
>>>>>      >>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
>>>>>      >>>> +
>>>>>      >>>> odp_timeout_free(odp_timeout_from_event(tt[i].ev));
>>>>>      >>>> +       }
>>>>>      >>>>         LOG_DBG("Thread %u: exiting\n", thr);
>>>>>      >>>>         return NULL;
>>>>>      >>>>  }
>>>>>      >>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>>>>>      >>>>         /* Destroy timer pool, all timers must have been freed
>>>>> */
>>>>>      >>>>         odp_timer_pool_destroy(tp);
>>>>>      >>
>>>>>      >>
>>>>>      >> Still need to set
>>>>>      >> (void)  odp_timer_pool_destroy(tp);
>>>>>      >> or
>>>>>      >> check the return code.
>>>>>      >> Else where in this file the explicit intention to ignore a
>>>>>      return code is
>>>>>      >> signaled with (void)
>>>>>      >>
>>>>>      >>
>>>>>      >>>>
>>>>>      >>>>
>>>>>      >>>> +       /* Destroy timeout pool, all timeouts must have been
>>>>>      freed */
>>>>>      >>>> +       int rc = odp_pool_destroy(tbp);
>>>>>      >>>> +       CU_ASSERT(rc == 0);
>>>>>      >>>> +
>>>>>      >>>>         CU_PASS("ODP timer test");
>>>>>      >>>>  }
>>>>>      >>>>
>>>>>      >>>> --
>>>>>      >>>> 1.9.1
>>>>>      >>>>
>>>>>      >>>>
>>>>>      >>>> _______________________________________________
>>>>>      >>>> lng-odp mailing list
>>>>>      >>>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>>>>      >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>      >>>
>>>>>      >>>
>>>>>      >>>
>>>>>      >>> _______________________________________________
>>>>>      >>> lng-odp mailing list
>>>>>      >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>>>>      >>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>      >>>
>>>>>      >>
>>>>>      >>
>>>>>      >>
>>>>>      >> --
>>>>>      >> Mike Holmes
>>>>>      >> Technical Manager - Linaro Networking Group
>>>>>      >> Linaro.org │ Open source software for ARM SoCs
>>>>>      >>
>>>>>      >>
>>>>>      >
>>>>>      >
>>>>>      >
>>>>>      > --
>>>>>      > Mike Holmes
>>>>>      > Technical Manager - Linaro Networking Group
>>>>>      > Linaro.org │ Open source software for ARM SoCs
>>>>>      >
>>>>>      >
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Mike Holmes
>>>>> Technical Manager - Linaro Networking Group
>>>>> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM
>>>>> SoCs
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> lng-odp mailing list
>>>>> lng-odp@lists.linaro.org
>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>
>>>>
>>>> _______________________________________________
>>>> lng-odp mailing list
>>>> lng-odp@lists.linaro.org
>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>>
>>>
>>> --
>>> Mike Holmes
>>> Technical Manager - Linaro Networking Group
>>> Linaro.org │ Open source software for ARM SoCs
>>>
>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Bill Fischofer March 6, 2015, 5:34 p.m. UTC | #14 
On Fri, Mar 6, 2015 at 11:14 AM, Maxim Uvarov <maxim.uvarov@linaro.org>
wrote:

> On 03/06/15 19:54, Ola Liljedahl wrote:
>
>> On 6 March 2015 at 17:36, Bill Fischofer <bill.fischofer@linaro.org>
>> wrote:
>>
>>> I don't think we need a rigid policy for everything.  The kernel
>>> guidelines
>>> are sufficient.  For variables that have limited scope, local definitions
>>> are already common.  The main consideration here is does it make for
>>> improved readability and maintainability. For variables used throughout a
>>> function the declare-at-the-top rule makes sense, but for temporaries,
>>> etc.
>>> I think more localized declaration can improve both.
>>>
>> I am all with Bill here.
>> Putting a use-once variable declaration at the top of the function far
>> away from where it is used just decreases the understanding.
>>
>> In general, I think it is much better to declare variables from the
>> point where you use them. The compiler will complain if you attempt to
>> redeclare the same name. Maybe compilers weren't so good back when
>> Linus started to code the Linux kernel but things have since moved on
>> and improved. Clang is even better with warnings.
>>
>
> I think by that they also wanted to say if you need to declare variable in
> the middle of
> the function then probably you need 2 functions.
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/
> linux.git/tree/Documentation/CodingStyle
> <snip>
>     Chapter 6: Functions
>
> Functions should be short and sweet, and do just one thing.  They should
> fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
> as we all know), and do one thing and do that well.
> ,,,,
> Another measure of the function is the number of local variables. They
> shouldn't exceed 5-10, or you're doing something wrong.  Re-think the
> function, and split it into smaller pieces.  A human brain can
> generally easily keep track of about 7 different things, anything more
> and it gets confused.  You know you're brilliant, but maybe you'd like
> to understand what you did 2 weeks from now.
> </snip>
>
> I used to see variables on top. And not things like "for (int i = 0;.... "
> I.e. more ANSI C and not C99.
>
> But might be there is some performance benefits, like better stack
> optimization
> if all variables declared on top.


Actually, you'd expect better stack optimization with more scoped local
variables.  For example, variables only used in local blocks can share
space with similar variables found in disjoint local blocks.  For
function-wide stuff, declaration order or placement of variables not in a
struct should be irrelevant.


>
>
> Maxim.
>
>
>  On Fri, Mar 6, 2015 at 9:56 AM, Mike Holmes <mike.holmes@linaro.org>
>>> wrote:
>>>
>>>>
>>>>
>>>> On 6 March 2015 at 10:49, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>>>>
>>>>> On 03/06/15 16:31, Mike Holmes wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On 6 March 2015 at 04:57, Ola Liljedahl <ola.liljedahl@linaro.org
>>>>>> <mailto:ola.liljedahl@linaro.org>> wrote:
>>>>>>
>>>>>>      OK fixed this in v3.
>>>>>>
>>>>>>      If this is a style rule
>>>>>>
>>>>>>  In CONTRIBUTING it's said that we are following kernel code style.
>>>>> But I did not find here requirement to define variables on top of the
>>>>> function:
>>>>> https://www.kernel.org/doc/Documentation/CodingStyle
>>>>>
>>>>>  From other point in kernel variables usually defined on not and very
>>>>> very
>>>>> rare
>>>>> defined in the function body.
>>>>>
>>>>
>>>>
>>>> Maybe we update our CONTRIBUTING, but I think our precedent is to
>>>> declare
>>>> variables at the top of the function, and to #define things at the
>>>> start of
>>>> a source .c file if  they are needed.
>>>>
>>>>  Maxim.
>>>>>
>>>>>
>>>>>       (and I approve of it, I just didn't expect
>>>>>>      odp_queue_destroy() to have any return value, I thought it was
>>>>>> succeed
>>>>>>      or crash semantics like I prefer), wouldn't it be good it there
>>>>>> was
>>>>>>      some way the compiler could always warn for this?
>>>>>>      GCC states: "The default is -Wunused-result" but it seems like
>>>>>> the
>>>>>>      function needs to be declared with the "warn_unused_result"
>>>>>> attribute.
>>>>>>      Is there any way to get this warning for all functions? If not,
>>>>>>      perhaps we should add this function attribute to ODP functions
>>>>>> such
>>>>>> as
>>>>>>      odp_queue_destroy()?
>>>>>>
>>>>>>
>>>>>> I am all for adding such hints, I am going to play with it and see how
>>>>>> it pans out.
>>>>>> If adding (void) suppress the warning in an application when you truly
>>>>>> don't want to look at the return code I see no harm in adding it to
>>>>>> the
>>>>>> whole API.
>>>>>>
>>>>>>
>>>>>>      On 5 March 2015 at 19:58, Mike Holmes <mike.holmes@linaro.org
>>>>>>      <mailto:mike.holmes@linaro.org>> wrote:
>>>>>>      > pasted to wrong on this v2 - meant odp_queue_destroy
>>>>>>      >
>>>>>>      > On 5 March 2015 at 13:57, Mike Holmes <mike.holmes@linaro.org
>>>>>>      <mailto:mike.holmes@linaro.org>> wrote:
>>>>>>      >>
>>>>>>      >>
>>>>>>      >>
>>>>>>      >> On 5 March 2015 at 10:57, Bill Fischofer
>>>>>>      <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>>>>>      >> wrote:
>>>>>>      >>>
>>>>>>      >>>
>>>>>>      >>>
>>>>>>      >>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl
>>>>>>      <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>>
>>>>>>      >>> wrote:
>>>>>>      >>>>
>>>>>>      >>>> Free queue and timeouts, destroy timeout pool before
>>>>>> termination.
>>>>>>      >>>> https://bugs.linaro.org/show_bug.cgi?id=1285
>>>>>>      >>>>
>>>>>>      >>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org
>>>>>>      <mailto:ola.liljedahl@linaro.org>>
>>>>>>      >>>
>>>>>>      >>>
>>>>>>      >>> Reviewed-and-tested-by: Bill Fischofer
>>>>>>      <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>>>>>
>>>>>>      >>>
>>>>>>      >>>>
>>>>>>      >>>> ---
>>>>>>      >>>> (This document/code contribution attached is provided under
>>>>>>      the terms of
>>>>>>      >>>> agreement LES-LTM-21309)
>>>>>>      >>>>
>>>>>>      >>>> Removed reference to Linaro CARDS as this is internal.
>>>>>>      >>>>
>>>>>>      >>>>  test/validation/odp_timer.c | 9 +++++++++
>>>>>>      >>>>  1 file changed, 9 insertions(+)
>>>>>>      >>>>
>>>>>>      >>>> diff --git a/test/validation/odp_timer.c
>>>>>>      b/test/validation/odp_timer.c
>>>>>>      >>>> index 88af61b..3e83367 100644
>>>>>>      >>>> --- a/test/validation/odp_timer.c
>>>>>>      >>>> +++ b/test/validation/odp_timer.c
>>>>>>      >>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void
>>>>>> *arg)
>>>>>>      >>>>         if (ev != ODP_EVENT_INVALID)
>>>>>>      >>>>                 CU_FAIL("Unexpected event received");
>>>>>>      >>>>
>>>>>>      >>>> +       odp_queue_destroy(queue);
>>>>>>      >>>> +       for (i = 0; i < NTIMERS; i++) {
>>>>>>      >>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
>>>>>>      >>>> +
>>>>>>      >>>> odp_timeout_free(odp_timeout_from_event(tt[i].ev));
>>>>>>      >>>> +       }
>>>>>>      >>>>         LOG_DBG("Thread %u: exiting\n", thr);
>>>>>>      >>>>         return NULL;
>>>>>>      >>>>  }
>>>>>>      >>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>>>>>>      >>>>         /* Destroy timer pool, all timers must have been
>>>>>> freed
>>>>>> */
>>>>>>      >>>>         odp_timer_pool_destroy(tp);
>>>>>>      >>
>>>>>>      >>
>>>>>>      >> Still need to set
>>>>>>      >> (void)  odp_timer_pool_destroy(tp);
>>>>>>      >> or
>>>>>>      >> check the return code.
>>>>>>      >> Else where in this file the explicit intention to ignore a
>>>>>>      return code is
>>>>>>      >> signaled with (void)
>>>>>>      >>
>>>>>>      >>
>>>>>>      >>>>
>>>>>>      >>>>
>>>>>>      >>>> +       /* Destroy timeout pool, all timeouts must have been
>>>>>>      freed */
>>>>>>      >>>> +       int rc = odp_pool_destroy(tbp);
>>>>>>      >>>> +       CU_ASSERT(rc == 0);
>>>>>>      >>>> +
>>>>>>      >>>>         CU_PASS("ODP timer test");
>>>>>>      >>>>  }
>>>>>>      >>>>
>>>>>>      >>>> --
>>>>>>      >>>> 1.9.1
>>>>>>      >>>>
>>>>>>      >>>>
>>>>>>      >>>> _______________________________________________
>>>>>>      >>>> lng-odp mailing list
>>>>>>      >>>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>>>>>      >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>>      >>>
>>>>>>      >>>
>>>>>>      >>>
>>>>>>      >>> _______________________________________________
>>>>>>      >>> lng-odp mailing list
>>>>>>      >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>>>>>      >>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>>      >>>
>>>>>>      >>
>>>>>>      >>
>>>>>>      >>
>>>>>>      >> --
>>>>>>      >> Mike Holmes
>>>>>>      >> Technical Manager - Linaro Networking Group
>>>>>>      >> Linaro.org │ Open source software for ARM SoCs
>>>>>>      >>
>>>>>>      >>
>>>>>>      >
>>>>>>      >
>>>>>>      >
>>>>>>      > --
>>>>>>      > Mike Holmes
>>>>>>      > Technical Manager - Linaro Networking Group
>>>>>>      > Linaro.org │ Open source software for ARM SoCs
>>>>>>      >
>>>>>>      >
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Mike Holmes
>>>>>> Technical Manager - Linaro Networking Group
>>>>>> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM
>>>>>> SoCs
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> lng-odp mailing list
>>>>>> lng-odp@lists.linaro.org
>>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> lng-odp mailing list
>>>>> lng-odp@lists.linaro.org
>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Mike Holmes
>>>> Technical Manager - Linaro Networking Group
>>>> Linaro.org │ Open source software for ARM SoCs
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> lng-odp mailing list
>>>> lng-odp@lists.linaro.org
>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>
>>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>>  _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
Ola Liljedahl March 6, 2015, 5:58 p.m. UTC | #15 
On 6 March 2015 at 18:14, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> On 03/06/15 19:54, Ola Liljedahl wrote:
>>
>> On 6 March 2015 at 17:36, Bill Fischofer <bill.fischofer@linaro.org>
>> wrote:
>>>
>>> I don't think we need a rigid policy for everything.  The kernel
>>> guidelines
>>> are sufficient.  For variables that have limited scope, local definitions
>>> are already common.  The main consideration here is does it make for
>>> improved readability and maintainability. For variables used throughout a
>>> function the declare-at-the-top rule makes sense, but for temporaries,
>>> etc.
>>> I think more localized declaration can improve both.
>>
>> I am all with Bill here.
>> Putting a use-once variable declaration at the top of the function far
>> away from where it is used just decreases the understanding.
>>
>> In general, I think it is much better to declare variables from the
>> point where you use them. The compiler will complain if you attempt to
>> redeclare the same name. Maybe compilers weren't so good back when
>> Linus started to code the Linux kernel but things have since moved on
>> and improved. Clang is even better with warnings.
>
>
> I think by that they also wanted to say if you need to declare variable in
> the middle of
> the function then probably you need 2 functions.
Due to the way (optimizing or not) compilers work, there isn't really
a need to declare variables in the middle of the function. This
language feature is solely to aid the programmer to understand the
actual scope of the variable. The compiler analyzes the source code
and understand the actual scope regardless.

If you follow your logic here, the function should be split into two
because I wanted to declare rc where it was used? This does not make
sense.

>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle
> <snip>
>     Chapter 6: Functions
>
> Functions should be short and sweet, and do just one thing.  They should
> fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
> as we all know), and do one thing and do that well.
> ,,,,
> Another measure of the function is the number of local variables. They
> shouldn't exceed 5-10, or you're doing something wrong.  Re-think the
> function, and split it into smaller pieces.  A human brain can
> generally easily keep track of about 7 different things, anything more
> and it gets confused.  You know you're brilliant, but maybe you'd like
> to understand what you did 2 weeks from now.
This another reason for keeping that local extremely temporary
variable away from the other long-lived variables that are declared at
the start of the function.
Essentially rc is not a (mutable) variable. It is assigned once and
then checked. rc could even be defined const (immutable). Or the code
could check the return value of odp_queue_destroy() without first
assigning it to rc (I think this code would be more complicated to
understand and debug, I try to avoid function calls with side effects
in conditional statements, some debuggers are not very good at
stepping into functions the way you want).

Declaring rc at the start of the function together with the other long
lived variables will increase the complexity of the code (there are
now more variables to keep track of).

> </snip>
>
> I used to see variables on top. And not things like "for (int i = 0;.... "
> I.e. more ANSI C and not C99.
Since the Linux kernel development started before the C99 standard was
defined, it is quite expected to not see much (or any) of the C99
features. Just changing to code the use C99 features for no good
reason is just asking for trouble (potentially destabilizing). But ODP
development was started after 1999 and requires a C99 compiler.

>
> But might be there is some performance benefits, like better stack
> optimization
> if all variables declared on top.
If compilers could *not* do live analysis of variables, then it would
probably be better to keep variable scopes as *small* as possible in
the code. If all variables in the program would be declared on the
outermost scope then the compiler would have to allocate space for all
of them, regardless of when the variables are actually used.


>
> Maxim.
>
>
>>> On Fri, Mar 6, 2015 at 9:56 AM, Mike Holmes <mike.holmes@linaro.org>
>>> wrote:
>>>>
>>>>
>>>>
>>>> On 6 March 2015 at 10:49, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>>>>>
>>>>> On 03/06/15 16:31, Mike Holmes wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 6 March 2015 at 04:57, Ola Liljedahl <ola.liljedahl@linaro.org
>>>>>> <mailto:ola.liljedahl@linaro.org>> wrote:
>>>>>>
>>>>>>      OK fixed this in v3.
>>>>>>
>>>>>>      If this is a style rule
>>>>>>
>>>>> In CONTRIBUTING it's said that we are following kernel code style.
>>>>> But I did not find here requirement to define variables on top of the
>>>>> function:
>>>>> https://www.kernel.org/doc/Documentation/CodingStyle
>>>>>
>>>>>  From other point in kernel variables usually defined on not and very
>>>>> very
>>>>> rare
>>>>> defined in the function body.
>>>>
>>>>
>>>>
>>>> Maybe we update our CONTRIBUTING, but I think our precedent is to
>>>> declare
>>>> variables at the top of the function, and to #define things at the start
>>>> of
>>>> a source .c file if  they are needed.
>>>>
>>>>> Maxim.
>>>>>
>>>>>
>>>>>>      (and I approve of it, I just didn't expect
>>>>>>      odp_queue_destroy() to have any return value, I thought it was
>>>>>> succeed
>>>>>>      or crash semantics like I prefer), wouldn't it be good it there
>>>>>> was
>>>>>>      some way the compiler could always warn for this?
>>>>>>      GCC states: "The default is -Wunused-result" but it seems like
>>>>>> the
>>>>>>      function needs to be declared with the "warn_unused_result"
>>>>>> attribute.
>>>>>>      Is there any way to get this warning for all functions? If not,
>>>>>>      perhaps we should add this function attribute to ODP functions
>>>>>> such
>>>>>> as
>>>>>>      odp_queue_destroy()?
>>>>>>
>>>>>>
>>>>>> I am all for adding such hints, I am going to play with it and see how
>>>>>> it pans out.
>>>>>> If adding (void) suppress the warning in an application when you truly
>>>>>> don't want to look at the return code I see no harm in adding it to
>>>>>> the
>>>>>> whole API.
>>>>>>
>>>>>>
>>>>>>      On 5 March 2015 at 19:58, Mike Holmes <mike.holmes@linaro.org
>>>>>>      <mailto:mike.holmes@linaro.org>> wrote:
>>>>>>      > pasted to wrong on this v2 - meant odp_queue_destroy
>>>>>>      >
>>>>>>      > On 5 March 2015 at 13:57, Mike Holmes <mike.holmes@linaro.org
>>>>>>      <mailto:mike.holmes@linaro.org>> wrote:
>>>>>>      >>
>>>>>>      >>
>>>>>>      >>
>>>>>>      >> On 5 March 2015 at 10:57, Bill Fischofer
>>>>>>      <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>>>>>      >> wrote:
>>>>>>      >>>
>>>>>>      >>>
>>>>>>      >>>
>>>>>>      >>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl
>>>>>>      <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>>
>>>>>>      >>> wrote:
>>>>>>      >>>>
>>>>>>      >>>> Free queue and timeouts, destroy timeout pool before
>>>>>> termination.
>>>>>>      >>>> https://bugs.linaro.org/show_bug.cgi?id=1285
>>>>>>      >>>>
>>>>>>      >>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org
>>>>>>      <mailto:ola.liljedahl@linaro.org>>
>>>>>>      >>>
>>>>>>      >>>
>>>>>>      >>> Reviewed-and-tested-by: Bill Fischofer
>>>>>>      <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>>>>>
>>>>>>      >>>
>>>>>>      >>>>
>>>>>>      >>>> ---
>>>>>>      >>>> (This document/code contribution attached is provided under
>>>>>>      the terms of
>>>>>>      >>>> agreement LES-LTM-21309)
>>>>>>      >>>>
>>>>>>      >>>> Removed reference to Linaro CARDS as this is internal.
>>>>>>      >>>>
>>>>>>      >>>>  test/validation/odp_timer.c | 9 +++++++++
>>>>>>      >>>>  1 file changed, 9 insertions(+)
>>>>>>      >>>>
>>>>>>      >>>> diff --git a/test/validation/odp_timer.c
>>>>>>      b/test/validation/odp_timer.c
>>>>>>      >>>> index 88af61b..3e83367 100644
>>>>>>      >>>> --- a/test/validation/odp_timer.c
>>>>>>      >>>> +++ b/test/validation/odp_timer.c
>>>>>>      >>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void
>>>>>> *arg)
>>>>>>      >>>>         if (ev != ODP_EVENT_INVALID)
>>>>>>      >>>>                 CU_FAIL("Unexpected event received");
>>>>>>      >>>>
>>>>>>      >>>> +       odp_queue_destroy(queue);
>>>>>>      >>>> +       for (i = 0; i < NTIMERS; i++) {
>>>>>>      >>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
>>>>>>      >>>> +
>>>>>>      >>>> odp_timeout_free(odp_timeout_from_event(tt[i].ev));
>>>>>>      >>>> +       }
>>>>>>      >>>>         LOG_DBG("Thread %u: exiting\n", thr);
>>>>>>      >>>>         return NULL;
>>>>>>      >>>>  }
>>>>>>      >>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>>>>>>      >>>>         /* Destroy timer pool, all timers must have been
>>>>>> freed
>>>>>> */
>>>>>>      >>>>         odp_timer_pool_destroy(tp);
>>>>>>      >>
>>>>>>      >>
>>>>>>      >> Still need to set
>>>>>>      >> (void)  odp_timer_pool_destroy(tp);
>>>>>>      >> or
>>>>>>      >> check the return code.
>>>>>>      >> Else where in this file the explicit intention to ignore a
>>>>>>      return code is
>>>>>>      >> signaled with (void)
>>>>>>      >>
>>>>>>      >>
>>>>>>      >>>>
>>>>>>      >>>>
>>>>>>      >>>> +       /* Destroy timeout pool, all timeouts must have been
>>>>>>      freed */
>>>>>>      >>>> +       int rc = odp_pool_destroy(tbp);
>>>>>>      >>>> +       CU_ASSERT(rc == 0);
>>>>>>      >>>> +
>>>>>>      >>>>         CU_PASS("ODP timer test");
>>>>>>      >>>>  }
>>>>>>      >>>>
>>>>>>      >>>> --
>>>>>>      >>>> 1.9.1
>>>>>>      >>>>
>>>>>>      >>>>
>>>>>>      >>>> _______________________________________________
>>>>>>      >>>> lng-odp mailing list
>>>>>>      >>>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>>>>>      >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>>      >>>
>>>>>>      >>>
>>>>>>      >>>
>>>>>>      >>> _______________________________________________
>>>>>>      >>> lng-odp mailing list
>>>>>>      >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>>>>>      >>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>>      >>>
>>>>>>      >>
>>>>>>      >>
>>>>>>      >>
>>>>>>      >> --
>>>>>>      >> Mike Holmes
>>>>>>      >> Technical Manager - Linaro Networking Group
>>>>>>      >> Linaro.org │ Open source software for ARM SoCs
>>>>>>      >>
>>>>>>      >>
>>>>>>      >
>>>>>>      >
>>>>>>      >
>>>>>>      > --
>>>>>>      > Mike Holmes
>>>>>>      > Technical Manager - Linaro Networking Group
>>>>>>      > Linaro.org │ Open source software for ARM SoCs
>>>>>>      >
>>>>>>      >
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Mike Holmes
>>>>>> Technical Manager - Linaro Networking Group
>>>>>> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM
>>>>>> SoCs
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> lng-odp mailing list
>>>>>> lng-odp@lists.linaro.org
>>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> lng-odp mailing list
>>>>> lng-odp@lists.linaro.org
>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Mike Holmes
>>>> Technical Manager - Linaro Networking Group
>>>> Linaro.org │ Open source software for ARM SoCs
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> lng-odp mailing list
>>>> lng-odp@lists.linaro.org
>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Ola Liljedahl March 9, 2015, 9:39 a.m. UTC | #16 
Funny you use C99 comments in an example were you promote we stick to
C89 language levels:-)


On 9 March 2015 at 10:05, Savolainen, Petri (Nokia - FI/Espoo)
<petri.savolainen@nokia.com> wrote:
> Hi,
>
> I think C89 / kernel style is flexible enough: you can declare variables (only) in the beginning of each scope block. I suggest we stick with the kernel style. The scope block is too large if a declaration at the  beginning of the block is too far away to be readable.
>
>
> void func(int x) {
>     int foo;
>     int i;
>
>     if (x) {
>         int a;      // OK
>         int b = 0;  // OK. Initialization can be combined with the declaration.
>         printf("hello");
>         int c;      // Not OK. Declaration after a block of code.
>         ...
>     }
>
>     for (i=0; i < x; i++) {
>         int a, b, c; // OK
>         ...
>     }
> }
>
>
> -Petri
>
>
>
>
>
>> -----Original Message-----
>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
>> bounces@lists.linaro.org] On Behalf Of ext Ola Liljedahl
>> Sent: Friday, March 06, 2015 7:59 PM
>> To: Maxim Uvarov
>> Cc: LNG ODP Mailman List
>> Subject: Re: [lng-odp] [PATCHv2] validation: odp_timer: cleanup for clean
>> termination
>>
>> On 6 March 2015 at 18:14, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>> > On 03/06/15 19:54, Ola Liljedahl wrote:
>> >>
>> >> On 6 March 2015 at 17:36, Bill Fischofer <bill.fischofer@linaro.org>
>> >> wrote:
>> >>>
>> >>> I don't think we need a rigid policy for everything.  The kernel
>> >>> guidelines
>> >>> are sufficient.  For variables that have limited scope, local
>> definitions
>> >>> are already common.  The main consideration here is does it make for
>> >>> improved readability and maintainability. For variables used
>> throughout a
>> >>> function the declare-at-the-top rule makes sense, but for temporaries,
>> >>> etc.
>> >>> I think more localized declaration can improve both.
>> >>
>> >> I am all with Bill here.
>> >> Putting a use-once variable declaration at the top of the function far
>> >> away from where it is used just decreases the understanding.
>> >>
>> >> In general, I think it is much better to declare variables from the
>> >> point where you use them. The compiler will complain if you attempt to
>> >> redeclare the same name. Maybe compilers weren't so good back when
>> >> Linus started to code the Linux kernel but things have since moved on
>> >> and improved. Clang is even better with warnings.
>> >
>> >
>> > I think by that they also wanted to say if you need to declare variable
>> in
>> > the middle of
>> > the function then probably you need 2 functions.
>> Due to the way (optimizing or not) compilers work, there isn't really
>> a need to declare variables in the middle of the function. This
>> language feature is solely to aid the programmer to understand the
>> actual scope of the variable. The compiler analyzes the source code
>> and understand the actual scope regardless.
>>
>> If you follow your logic here, the function should be split into two
>> because I wanted to declare rc where it was used? This does not make
>> sense.
>>
>> >
>> >
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Docum
>> entation/CodingStyle
>> > <snip>
>> >     Chapter 6: Functions
>> >
>> > Functions should be short and sweet, and do just one thing.  They should
>> > fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
>> > as we all know), and do one thing and do that well.
>> > ,,,,
>> > Another measure of the function is the number of local variables. They
>> > shouldn't exceed 5-10, or you're doing something wrong.  Re-think the
>> > function, and split it into smaller pieces.  A human brain can
>> > generally easily keep track of about 7 different things, anything more
>> > and it gets confused.  You know you're brilliant, but maybe you'd like
>> > to understand what you did 2 weeks from now.
>> This another reason for keeping that local extremely temporary
>> variable away from the other long-lived variables that are declared at
>> the start of the function.
>> Essentially rc is not a (mutable) variable. It is assigned once and
>> then checked. rc could even be defined const (immutable). Or the code
>> could check the return value of odp_queue_destroy() without first
>> assigning it to rc (I think this code would be more complicated to
>> understand and debug, I try to avoid function calls with side effects
>> in conditional statements, some debuggers are not very good at
>> stepping into functions the way you want).
>>
>> Declaring rc at the start of the function together with the other long
>> lived variables will increase the complexity of the code (there are
>> now more variables to keep track of).
>>
>> > </snip>
>> >
>> > I used to see variables on top. And not things like "for (int i = 0;....
>> "
>> > I.e. more ANSI C and not C99.
>> Since the Linux kernel development started before the C99 standard was
>> defined, it is quite expected to not see much (or any) of the C99
>> features. Just changing to code the use C99 features for no good
>> reason is just asking for trouble (potentially destabilizing). But ODP
>> development was started after 1999 and requires a C99 compiler.
>>
>> >
>> > But might be there is some performance benefits, like better stack
>> > optimization
>> > if all variables declared on top.
>> If compilers could *not* do live analysis of variables, then it would
>> probably be better to keep variable scopes as *small* as possible in
>> the code. If all variables in the program would be declared on the
>> outermost scope then the compiler would have to allocate space for all
>> of them, regardless of when the variables are actually used.
>>
>>
>> >
>> > Maxim.
>> >
>> >
>> >>> On Fri, Mar 6, 2015 at 9:56 AM, Mike Holmes <mike.holmes@linaro.org>
>> >>> wrote:
>> >>>>
>> >>>>
>> >>>>
>> >>>> On 6 March 2015 at 10:49, Maxim Uvarov <maxim.uvarov@linaro.org>
>> wrote:
>> >>>>>
>> >>>>> On 03/06/15 16:31, Mike Holmes wrote:
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>>>>> On 6 March 2015 at 04:57, Ola Liljedahl <ola.liljedahl@linaro.org
>> >>>>>> <mailto:ola.liljedahl@linaro.org>> wrote:
>> >>>>>>
>> >>>>>>      OK fixed this in v3.
>> >>>>>>
>> >>>>>>      If this is a style rule
>> >>>>>>
>> >>>>> In CONTRIBUTING it's said that we are following kernel code style.
>> >>>>> But I did not find here requirement to define variables on top of
>> the
>> >>>>> function:
>> >>>>> https://www.kernel.org/doc/Documentation/CodingStyle
>> >>>>>
>> >>>>>  From other point in kernel variables usually defined on not and
>> very
>> >>>>> very
>> >>>>> rare
>> >>>>> defined in the function body.
>> >>>>
>> >>>>
>> >>>>
>> >>>> Maybe we update our CONTRIBUTING, but I think our precedent is to
>> >>>> declare
>> >>>> variables at the top of the function, and to #define things at the
>> start
>> >>>> of
>> >>>> a source .c file if  they are needed.
>> >>>>
>> >>>>> Maxim.
>> >>>>>
>> >>>>>
>> >>>>>>      (and I approve of it, I just didn't expect
>> >>>>>>      odp_queue_destroy() to have any return value, I thought it was
>> >>>>>> succeed
>> >>>>>>      or crash semantics like I prefer), wouldn't it be good it
>> there
>> >>>>>> was
>> >>>>>>      some way the compiler could always warn for this?
>> >>>>>>      GCC states: "The default is -Wunused-result" but it seems like
>> >>>>>> the
>> >>>>>>      function needs to be declared with the "warn_unused_result"
>> >>>>>> attribute.
>> >>>>>>      Is there any way to get this warning for all functions? If
>> not,
>> >>>>>>      perhaps we should add this function attribute to ODP functions
>> >>>>>> such
>> >>>>>> as
>> >>>>>>      odp_queue_destroy()?
>> >>>>>>
>> >>>>>>
>> >>>>>> I am all for adding such hints, I am going to play with it and see
>> how
>> >>>>>> it pans out.
>> >>>>>> If adding (void) suppress the warning in an application when you
>> truly
>> >>>>>> don't want to look at the return code I see no harm in adding it to
>> >>>>>> the
>> >>>>>> whole API.
>> >>>>>>
>> >>>>>>
>> >>>>>>      On 5 March 2015 at 19:58, Mike Holmes <mike.holmes@linaro.org
>> >>>>>>      <mailto:mike.holmes@linaro.org>> wrote:
>> >>>>>>      > pasted to wrong on this v2 - meant odp_queue_destroy
>> >>>>>>      >
>> >>>>>>      > On 5 March 2015 at 13:57, Mike Holmes
>> <mike.holmes@linaro.org
>> >>>>>>      <mailto:mike.holmes@linaro.org>> wrote:
>> >>>>>>      >>
>> >>>>>>      >>
>> >>>>>>      >>
>> >>>>>>      >> On 5 March 2015 at 10:57, Bill Fischofer
>> >>>>>>      <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>> >>>>>>      >> wrote:
>> >>>>>>      >>>
>> >>>>>>      >>>
>> >>>>>>      >>>
>> >>>>>>      >>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl
>> >>>>>>      <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>>
>> >>>>>>      >>> wrote:
>> >>>>>>      >>>>
>> >>>>>>      >>>> Free queue and timeouts, destroy timeout pool before
>> >>>>>> termination.
>> >>>>>>      >>>> https://bugs.linaro.org/show_bug.cgi?id=1285
>> >>>>>>      >>>>
>> >>>>>>      >>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org
>> >>>>>>      <mailto:ola.liljedahl@linaro.org>>
>> >>>>>>      >>>
>> >>>>>>      >>>
>> >>>>>>      >>> Reviewed-and-tested-by: Bill Fischofer
>> >>>>>>      <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>> >>>>>>
>> >>>>>>      >>>
>> >>>>>>      >>>>
>> >>>>>>      >>>> ---
>> >>>>>>      >>>> (This document/code contribution attached is provided
>> under
>> >>>>>>      the terms of
>> >>>>>>      >>>> agreement LES-LTM-21309)
>> >>>>>>      >>>>
>> >>>>>>      >>>> Removed reference to Linaro CARDS as this is internal.
>> >>>>>>      >>>>
>> >>>>>>      >>>>  test/validation/odp_timer.c | 9 +++++++++
>> >>>>>>      >>>>  1 file changed, 9 insertions(+)
>> >>>>>>      >>>>
>> >>>>>>      >>>> diff --git a/test/validation/odp_timer.c
>> >>>>>>      b/test/validation/odp_timer.c
>> >>>>>>      >>>> index 88af61b..3e83367 100644
>> >>>>>>      >>>> --- a/test/validation/odp_timer.c
>> >>>>>>      >>>> +++ b/test/validation/odp_timer.c
>> >>>>>>      >>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void
>> >>>>>> *arg)
>> >>>>>>      >>>>         if (ev != ODP_EVENT_INVALID)
>> >>>>>>      >>>>                 CU_FAIL("Unexpected event received");
>> >>>>>>      >>>>
>> >>>>>>      >>>> +       odp_queue_destroy(queue);
>> >>>>>>      >>>> +       for (i = 0; i < NTIMERS; i++) {
>> >>>>>>      >>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
>> >>>>>>      >>>> +
>> >>>>>>      >>>> odp_timeout_free(odp_timeout_from_event(tt[i].ev));
>> >>>>>>      >>>> +       }
>> >>>>>>      >>>>         LOG_DBG("Thread %u: exiting\n", thr);
>> >>>>>>      >>>>         return NULL;
>> >>>>>>      >>>>  }
>> >>>>>>      >>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>> >>>>>>      >>>>         /* Destroy timer pool, all timers must have been
>> >>>>>> freed
>> >>>>>> */
>> >>>>>>      >>>>         odp_timer_pool_destroy(tp);
>> >>>>>>      >>
>> >>>>>>      >>
>> >>>>>>      >> Still need to set
>> >>>>>>      >> (void)  odp_timer_pool_destroy(tp);
>> >>>>>>      >> or
>> >>>>>>      >> check the return code.
>> >>>>>>      >> Else where in this file the explicit intention to ignore a
>> >>>>>>      return code is
>> >>>>>>      >> signaled with (void)
>> >>>>>>      >>
>> >>>>>>      >>
>> >>>>>>      >>>>
>> >>>>>>      >>>>
>> >>>>>>      >>>> +       /* Destroy timeout pool, all timeouts must have
>> been
>> >>>>>>      freed */
>> >>>>>>      >>>> +       int rc = odp_pool_destroy(tbp);
>> >>>>>>      >>>> +       CU_ASSERT(rc == 0);
>> >>>>>>      >>>> +
>> >>>>>>      >>>>         CU_PASS("ODP timer test");
>> >>>>>>      >>>>  }
>> >>>>>>      >>>>
>> >>>>>>      >>>> --
>> >>>>>>      >>>> 1.9.1
>> >>>>>>      >>>>
>> >>>>>>      >>>>
>> >>>>>>      >>>> _______________________________________________
>> >>>>>>      >>>> lng-odp mailing list
>> >>>>>>      >>>> lng-odp@lists.linaro.org <mailto:lng-
>> odp@lists.linaro.org>
>> >>>>>>      >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>> >>>>>>      >>>
>> >>>>>>      >>>
>> >>>>>>      >>>
>> >>>>>>      >>> _______________________________________________
>> >>>>>>      >>> lng-odp mailing list
>> >>>>>>      >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>> >>>>>>      >>> http://lists.linaro.org/mailman/listinfo/lng-odp
>> >>>>>>      >>>
>> >>>>>>      >>
>> >>>>>>      >>
>> >>>>>>      >>
>> >>>>>>      >> --
>> >>>>>>      >> Mike Holmes
>> >>>>>>      >> Technical Manager - Linaro Networking Group
>> >>>>>>      >> Linaro.org │ Open source software for ARM SoCs
>> >>>>>>      >>
>> >>>>>>      >>
>> >>>>>>      >
>> >>>>>>      >
>> >>>>>>      >
>> >>>>>>      > --
>> >>>>>>      > Mike Holmes
>> >>>>>>      > Technical Manager - Linaro Networking Group
>> >>>>>>      > Linaro.org │ Open source software for ARM SoCs
>> >>>>>>      >
>> >>>>>>      >
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>>>>> --
>> >>>>>> Mike Holmes
>> >>>>>> Technical Manager - Linaro Networking Group
>> >>>>>> Linaro.org <http://www.linaro.org/>***│ *Open source software for
>> ARM
>> >>>>>> SoCs
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>>>>> _______________________________________________
>> >>>>>> lng-odp mailing list
>> >>>>>> lng-odp@lists.linaro.org
>> >>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>> >>>>>
>> >>>>>
>> >>>>>
>> >>>>> _______________________________________________
>> >>>>> lng-odp mailing list
>> >>>>> lng-odp@lists.linaro.org
>> >>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>> >>>>
>> >>>>
>> >>>>
>> >>>>
>> >>>> --
>> >>>> Mike Holmes
>> >>>> Technical Manager - Linaro Networking Group
>> >>>> Linaro.org │ Open source software for ARM SoCs
>> >>>>
>> >>>>
>> >>>>
>> >>>> _______________________________________________
>> >>>> lng-odp mailing list
>> >>>> lng-odp@lists.linaro.org
>> >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>> >>>>
>> >>>
>> >>> _______________________________________________
>> >>> lng-odp mailing list
>> >>> lng-odp@lists.linaro.org
>> >>> http://lists.linaro.org/mailman/listinfo/lng-odp
>> >>>
>> >> _______________________________________________
>> >> lng-odp mailing list
>> >> lng-odp@lists.linaro.org
>> >> http://lists.linaro.org/mailman/listinfo/lng-odp
>> >
>> >
>> >
>> > _______________________________________________
>> > lng-odp mailing list
>> > lng-odp@lists.linaro.org
>> > http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
Maxim Uvarov March 10, 2015, 8:52 a.m. UTC | #17 
On 03/09/15 12:05, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> Hi,
>
> I think C89 / kernel style is flexible enough: you can declare variables (only) in the beginning of each scope block. I suggest we stick with the kernel style. The scope block is too large if a declaration at the  beginning of the block is too far away to be readable.
>
>
> void func(int x) {
>      int foo;
>      int i;
>
>      if (x) {
>          int a;      // OK
>          int b = 0;  // OK. Initialization can be combined with the declaration.
>          printf("hello");
>          int c;      // Not OK. Declaration after a block of code.
>          ...
>      }
>
>      for (i=0; i < x; i++) {
>          int a, b, c; // OK
>          ...
>      }
Most probably in first and second case it's better to create separate 
function. That should increase readability.

Modern gcc allows a lot of things. Even nested functions like that:
main()
{
         int i = 0;
         int x(int y)
         {
                 int x1(int z)
                 {
                         return z+i;
                 }

                 return x1(i) + i;
         }
         printf("i %d\n", x(i));
}


But for me it's very hard to read. However in some cases that solution 
can be elegant....

So I still think that we need to go with small and simple functions with 
variables definition on top. Might be small assumption if it's very 
reasonable with code like Petri wrote above (define in the beginning of 
block).

Thanks,
Maxim.

> }
>
>
> -Petri
>
>
>
>
>
>> -----Original Message-----
>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
>> bounces@lists.linaro.org] On Behalf Of ext Ola Liljedahl
>> Sent: Friday, March 06, 2015 7:59 PM
>> To: Maxim Uvarov
>> Cc: LNG ODP Mailman List
>> Subject: Re: [lng-odp] [PATCHv2] validation: odp_timer: cleanup for clean
>> termination
>>
>> On 6 March 2015 at 18:14, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>>> On 03/06/15 19:54, Ola Liljedahl wrote:
>>>> On 6 March 2015 at 17:36, Bill Fischofer <bill.fischofer@linaro.org>
>>>> wrote:
>>>>> I don't think we need a rigid policy for everything.  The kernel
>>>>> guidelines
>>>>> are sufficient.  For variables that have limited scope, local
>> definitions
>>>>> are already common.  The main consideration here is does it make for
>>>>> improved readability and maintainability. For variables used
>> throughout a
>>>>> function the declare-at-the-top rule makes sense, but for temporaries,
>>>>> etc.
>>>>> I think more localized declaration can improve both.
>>>> I am all with Bill here.
>>>> Putting a use-once variable declaration at the top of the function far
>>>> away from where it is used just decreases the understanding.
>>>>
>>>> In general, I think it is much better to declare variables from the
>>>> point where you use them. The compiler will complain if you attempt to
>>>> redeclare the same name. Maybe compilers weren't so good back when
>>>> Linus started to code the Linux kernel but things have since moved on
>>>> and improved. Clang is even better with warnings.
>>>
>>> I think by that they also wanted to say if you need to declare variable
>> in
>>> the middle of
>>> the function then probably you need 2 functions.
>> Due to the way (optimizing or not) compilers work, there isn't really
>> a need to declare variables in the middle of the function. This
>> language feature is solely to aid the programmer to understand the
>> actual scope of the variable. The compiler analyzes the source code
>> and understand the actual scope regardless.
>>
>> If you follow your logic here, the function should be split into two
>> because I wanted to declare rc where it was used? This does not make
>> sense.
>>
>>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Docum
>> entation/CodingStyle
>>> <snip>
>>>      Chapter 6: Functions
>>>
>>> Functions should be short and sweet, and do just one thing.  They should
>>> fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
>>> as we all know), and do one thing and do that well.
>>> ,,,,
>>> Another measure of the function is the number of local variables. They
>>> shouldn't exceed 5-10, or you're doing something wrong.  Re-think the
>>> function, and split it into smaller pieces.  A human brain can
>>> generally easily keep track of about 7 different things, anything more
>>> and it gets confused.  You know you're brilliant, but maybe you'd like
>>> to understand what you did 2 weeks from now.
>> This another reason for keeping that local extremely temporary
>> variable away from the other long-lived variables that are declared at
>> the start of the function.
>> Essentially rc is not a (mutable) variable. It is assigned once and
>> then checked. rc could even be defined const (immutable). Or the code
>> could check the return value of odp_queue_destroy() without first
>> assigning it to rc (I think this code would be more complicated to
>> understand and debug, I try to avoid function calls with side effects
>> in conditional statements, some debuggers are not very good at
>> stepping into functions the way you want).
>>
>> Declaring rc at the start of the function together with the other long
>> lived variables will increase the complexity of the code (there are
>> now more variables to keep track of).
>>
>>> </snip>
>>>
>>> I used to see variables on top. And not things like "for (int i = 0;....
>> "
>>> I.e. more ANSI C and not C99.
>> Since the Linux kernel development started before the C99 standard was
>> defined, it is quite expected to not see much (or any) of the C99
>> features. Just changing to code the use C99 features for no good
>> reason is just asking for trouble (potentially destabilizing). But ODP
>> development was started after 1999 and requires a C99 compiler.
>>
>>> But might be there is some performance benefits, like better stack
>>> optimization
>>> if all variables declared on top.
>> If compilers could *not* do live analysis of variables, then it would
>> probably be better to keep variable scopes as *small* as possible in
>> the code. If all variables in the program would be declared on the
>> outermost scope then the compiler would have to allocate space for all
>> of them, regardless of when the variables are actually used.
>>
>>
>>> Maxim.
>>>
>>>
>>>>> On Fri, Mar 6, 2015 at 9:56 AM, Mike Holmes <mike.holmes@linaro.org>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> On 6 March 2015 at 10:49, Maxim Uvarov <maxim.uvarov@linaro.org>
>> wrote:
>>>>>>> On 03/06/15 16:31, Mike Holmes wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6 March 2015 at 04:57, Ola Liljedahl <ola.liljedahl@linaro.org
>>>>>>>> <mailto:ola.liljedahl@linaro.org>> wrote:
>>>>>>>>
>>>>>>>>       OK fixed this in v3.
>>>>>>>>
>>>>>>>>       If this is a style rule
>>>>>>>>
>>>>>>> In CONTRIBUTING it's said that we are following kernel code style.
>>>>>>> But I did not find here requirement to define variables on top of
>> the
>>>>>>> function:
>>>>>>> https://www.kernel.org/doc/Documentation/CodingStyle
>>>>>>>
>>>>>>>   From other point in kernel variables usually defined on not and
>> very
>>>>>>> very
>>>>>>> rare
>>>>>>> defined in the function body.
>>>>>>
>>>>>>
>>>>>> Maybe we update our CONTRIBUTING, but I think our precedent is to
>>>>>> declare
>>>>>> variables at the top of the function, and to #define things at the
>> start
>>>>>> of
>>>>>> a source .c file if  they are needed.
>>>>>>
>>>>>>> Maxim.
>>>>>>>
>>>>>>>
>>>>>>>>       (and I approve of it, I just didn't expect
>>>>>>>>       odp_queue_destroy() to have any return value, I thought it was
>>>>>>>> succeed
>>>>>>>>       or crash semantics like I prefer), wouldn't it be good it
>> there
>>>>>>>> was
>>>>>>>>       some way the compiler could always warn for this?
>>>>>>>>       GCC states: "The default is -Wunused-result" but it seems like
>>>>>>>> the
>>>>>>>>       function needs to be declared with the "warn_unused_result"
>>>>>>>> attribute.
>>>>>>>>       Is there any way to get this warning for all functions? If
>> not,
>>>>>>>>       perhaps we should add this function attribute to ODP functions
>>>>>>>> such
>>>>>>>> as
>>>>>>>>       odp_queue_destroy()?
>>>>>>>>
>>>>>>>>
>>>>>>>> I am all for adding such hints, I am going to play with it and see
>> how
>>>>>>>> it pans out.
>>>>>>>> If adding (void) suppress the warning in an application when you
>> truly
>>>>>>>> don't want to look at the return code I see no harm in adding it to
>>>>>>>> the
>>>>>>>> whole API.
>>>>>>>>
>>>>>>>>
>>>>>>>>       On 5 March 2015 at 19:58, Mike Holmes <mike.holmes@linaro.org
>>>>>>>>       <mailto:mike.holmes@linaro.org>> wrote:
>>>>>>>>       > pasted to wrong on this v2 - meant odp_queue_destroy
>>>>>>>>       >
>>>>>>>>       > On 5 March 2015 at 13:57, Mike Holmes
>> <mike.holmes@linaro.org
>>>>>>>>       <mailto:mike.holmes@linaro.org>> wrote:
>>>>>>>>       >>
>>>>>>>>       >>
>>>>>>>>       >>
>>>>>>>>       >> On 5 March 2015 at 10:57, Bill Fischofer
>>>>>>>>       <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>>>>>>>       >> wrote:
>>>>>>>>       >>>
>>>>>>>>       >>>
>>>>>>>>       >>>
>>>>>>>>       >>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl
>>>>>>>>       <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>>
>>>>>>>>       >>> wrote:
>>>>>>>>       >>>>
>>>>>>>>       >>>> Free queue and timeouts, destroy timeout pool before
>>>>>>>> termination.
>>>>>>>>       >>>> https://bugs.linaro.org/show_bug.cgi?id=1285
>>>>>>>>       >>>>
>>>>>>>>       >>>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org
>>>>>>>>       <mailto:ola.liljedahl@linaro.org>>
>>>>>>>>       >>>
>>>>>>>>       >>>
>>>>>>>>       >>> Reviewed-and-tested-by: Bill Fischofer
>>>>>>>>       <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>>
>>>>>>>>
>>>>>>>>       >>>
>>>>>>>>       >>>>
>>>>>>>>       >>>> ---
>>>>>>>>       >>>> (This document/code contribution attached is provided
>> under
>>>>>>>>       the terms of
>>>>>>>>       >>>> agreement LES-LTM-21309)
>>>>>>>>       >>>>
>>>>>>>>       >>>> Removed reference to Linaro CARDS as this is internal.
>>>>>>>>       >>>>
>>>>>>>>       >>>>  test/validation/odp_timer.c | 9 +++++++++
>>>>>>>>       >>>>  1 file changed, 9 insertions(+)
>>>>>>>>       >>>>
>>>>>>>>       >>>> diff --git a/test/validation/odp_timer.c
>>>>>>>>       b/test/validation/odp_timer.c
>>>>>>>>       >>>> index 88af61b..3e83367 100644
>>>>>>>>       >>>> --- a/test/validation/odp_timer.c
>>>>>>>>       >>>> +++ b/test/validation/odp_timer.c
>>>>>>>>       >>>> @@ -336,6 +336,11 @@ static void *worker_entrypoint(void
>>>>>>>> *arg)
>>>>>>>>       >>>>         if (ev != ODP_EVENT_INVALID)
>>>>>>>>       >>>>                 CU_FAIL("Unexpected event received");
>>>>>>>>       >>>>
>>>>>>>>       >>>> +       odp_queue_destroy(queue);
>>>>>>>>       >>>> +       for (i = 0; i < NTIMERS; i++) {
>>>>>>>>       >>>> +               if (tt[i].ev != ODP_EVENT_INVALID)
>>>>>>>>       >>>> +
>>>>>>>>       >>>> odp_timeout_free(odp_timeout_from_event(tt[i].ev));
>>>>>>>>       >>>> +       }
>>>>>>>>       >>>>         LOG_DBG("Thread %u: exiting\n", thr);
>>>>>>>>       >>>>         return NULL;
>>>>>>>>       >>>>  }
>>>>>>>>       >>>> @@ -426,6 +431,10 @@ static void test_odp_timer_all(void)
>>>>>>>>       >>>>         /* Destroy timer pool, all timers must have been
>>>>>>>> freed
>>>>>>>> */
>>>>>>>>       >>>>         odp_timer_pool_destroy(tp);
>>>>>>>>       >>
>>>>>>>>       >>
>>>>>>>>       >> Still need to set
>>>>>>>>       >> (void)  odp_timer_pool_destroy(tp);
>>>>>>>>       >> or
>>>>>>>>       >> check the return code.
>>>>>>>>       >> Else where in this file the explicit intention to ignore a
>>>>>>>>       return code is
>>>>>>>>       >> signaled with (void)
>>>>>>>>       >>
>>>>>>>>       >>
>>>>>>>>       >>>>
>>>>>>>>       >>>>
>>>>>>>>       >>>> +       /* Destroy timeout pool, all timeouts must have
>> been
>>>>>>>>       freed */
>>>>>>>>       >>>> +       int rc = odp_pool_destroy(tbp);
>>>>>>>>       >>>> +       CU_ASSERT(rc == 0);
>>>>>>>>       >>>> +
>>>>>>>>       >>>>         CU_PASS("ODP timer test");
>>>>>>>>       >>>>  }
>>>>>>>>       >>>>
>>>>>>>>       >>>> --
>>>>>>>>       >>>> 1.9.1
>>>>>>>>       >>>>
>>>>>>>>       >>>>
>>>>>>>>       >>>> _______________________________________________
>>>>>>>>       >>>> lng-odp mailing list
>>>>>>>>       >>>> lng-odp@lists.linaro.org <mailto:lng-
>> odp@lists.linaro.org>
>>>>>>>>       >>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>>>>       >>>
>>>>>>>>       >>>
>>>>>>>>       >>>
>>>>>>>>       >>> _______________________________________________
>>>>>>>>       >>> lng-odp mailing list
>>>>>>>>       >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>>>>>>>       >>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>>>>       >>>
>>>>>>>>       >>
>>>>>>>>       >>
>>>>>>>>       >>
>>>>>>>>       >> --
>>>>>>>>       >> Mike Holmes
>>>>>>>>       >> Technical Manager - Linaro Networking Group
>>>>>>>>       >> Linaro.org │ Open source software for ARM SoCs
>>>>>>>>       >>
>>>>>>>>       >>
>>>>>>>>       >
>>>>>>>>       >
>>>>>>>>       >
>>>>>>>>       > --
>>>>>>>>       > Mike Holmes
>>>>>>>>       > Technical Manager - Linaro Networking Group
>>>>>>>>       > Linaro.org │ Open source software for ARM SoCs
>>>>>>>>       >
>>>>>>>>       >
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Mike Holmes
>>>>>>>> Technical Manager - Linaro Networking Group
>>>>>>>> Linaro.org <http://www.linaro.org/>***│ *Open source software for
>> ARM
>>>>>>>> SoCs
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> lng-odp mailing list
>>>>>>>> lng-odp@lists.linaro.org
>>>>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> lng-odp mailing list
>>>>>>> lng-odp@lists.linaro.org
>>>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Mike Holmes
>>>>>> Technical Manager - Linaro Networking Group
>>>>>> Linaro.org │ Open source software for ARM SoCs
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> lng-odp mailing list
>>>>>> lng-odp@lists.linaro.org
>>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>>
>>>>> _______________________________________________
>>>>> lng-odp mailing list
>>>>> lng-odp@lists.linaro.org
>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>>
>>>> _______________________________________________
>>>> lng-odp mailing list
>>>> lng-odp@lists.linaro.org
>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
diff mbox

Patch

diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c
index 88af61b..3e83367 100644
--- a/test/validation/odp_timer.c
+++ b/test/validation/odp_timer.c
@@ -336,6 +336,11 @@  static void *worker_entrypoint(void *arg)
 	if (ev != ODP_EVENT_INVALID)
 		CU_FAIL("Unexpected event received");
 
+	odp_queue_destroy(queue);
+	for (i = 0; i < NTIMERS; i++) {
+		if (tt[i].ev != ODP_EVENT_INVALID)
+			odp_timeout_free(odp_timeout_from_event(tt[i].ev));
+	}
 	LOG_DBG("Thread %u: exiting\n", thr);
 	return NULL;
 }
@@ -426,6 +431,10 @@  static void test_odp_timer_all(void)
 	/* Destroy timer pool, all timers must have been freed */
 	odp_timer_pool_destroy(tp);
 
+	/* Destroy timeout pool, all timeouts must have been freed */
+	int rc = odp_pool_destroy(tbp);
+	CU_ASSERT(rc == 0);
+
 	CU_PASS("ODP timer test");
 }