On 6 March 2015 at 12:03, Bill Fischofer <[email protected]> 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 <[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 >> >> >> > -- 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
