On 6 March 2015 at 17:36, Bill Fischofer <[email protected]> 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 <[email protected]> wrote: >> >> >> >> On 6 March 2015 at 10:49, Maxim Uvarov <[email protected]> wrote: >>> >>> On 03/06/15 16:31, Mike Holmes wrote: >>>> >>>> >>>> >>>> On 6 March 2015 at 04:57, Ola Liljedahl <[email protected] >>>> <mailto:[email protected]>> 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 <[email protected] >>>> <mailto:[email protected]>> wrote: >>>> > pasted to wrong on this v2 - meant odp_queue_destroy >>>> > >>>> > On 5 March 2015 at 13:57, Mike Holmes <[email protected] >>>> <mailto:[email protected]>> wrote: >>>> >> >>>> >> >>>> >> >>>> >> On 5 March 2015 at 10:57, Bill Fischofer >>>> <[email protected] <mailto:[email protected]>> >>>> >> wrote: >>>> >>> >>>> >>> >>>> >>> >>>> >>> On Thu, Mar 5, 2015 at 3:47 AM, Ola Liljedahl >>>> <[email protected] <mailto:[email protected]>> >>>> >>> wrote: >>>> >>>> >>>> >>>> Free queue and timeouts, destroy timeout pool before >>>> termination. >>>> >>>> https://bugs.linaro.org/show_bug.cgi?id=1285 >>>> >>>> >>>> >>>> Signed-off-by: Ola Liljedahl <[email protected] >>>> <mailto:[email protected]>> >>>> >>> >>>> >>> >>>> >>> Reviewed-and-tested-by: Bill Fischofer >>>> <[email protected] <mailto:[email protected]>> >>>> >>>> >>> >>>> >>>> >>>> >>>> --- >>>> >>>> (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 >>>> >>>> [email protected] <mailto:[email protected]> >>>> >>>> http://lists.linaro.org/mailman/listinfo/lng-odp >>>> >>> >>>> >>> >>>> >>> >>>> >>> _______________________________________________ >>>> >>> lng-odp mailing list >>>> >>> [email protected] <mailto:[email protected]> >>>> >>> 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 >>>> [email protected] >>>> http://lists.linaro.org/mailman/listinfo/lng-odp >>> >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> [email protected] >>> 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 >> [email protected] >> http://lists.linaro.org/mailman/listinfo/lng-odp >> > > > _______________________________________________ > lng-odp mailing list > [email protected] > http://lists.linaro.org/mailman/listinfo/lng-odp > _______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
