On 6 March 2015 at 18:14, Maxim Uvarov <[email protected]> wrote: > On 03/06/15 19:54, Ola Liljedahl 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 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 <[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 > > > > _______________________________________________ > 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
