I think our policy is: Use good judgement in everything. :) On Fri, Mar 6, 2015 at 11:02 AM, Mike Holmes <[email protected]> wrote:
> > > 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
