On 6 March 2015 at 11:54, Ola Liljedahl <[email protected]> wrote:
> 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. > 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 <[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 > > > -- 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
