Also, in viewing the util.h code, I saw, extern int (*build_assert(void))[1];
want to ask: what is the "(*build_assert(void))" ? Thanks, On Mon, Aug 26, 2013 at 1:24 PM, Alex Wang <al...@nicira.com> wrote: > Thanks Ben for the review, > > > > +static long long int coverage_run_time = LLONG_MIN; >> > + >> > +/* Defines the moving average array index variables. */ >> > +static unsigned int min_idx = 0; >> > +static unsigned int hr_idx = 0; >> >> It looks to me that min_idx and hr_idx are used only in >> coverage_run(). You might consider moving them into that function. >> >> > > Makes sense, I'll adjust this. > > > >> > +/* Index counter used to compute the moving average array's index. */ >> > +static unsigned int idx_count = 0; >> >> It looks to me like idx_count should be protected by coverage_mutex. > > > > I'm a bit confused here. Want to ask, that there will only be one thread > going through the loop in ovs-vswitchd.c (which calls coverage_run()), > right? If so, idx_count will only be accessed by that thread. Then do we > still need to protect it? > > > > >> > + coverage_array_sum(c[i]->min, MIN_AVG_LEN), >> > + coverage_array_sum(c[i]->hr, HR_AVG_LEN), >> > + totals[i])); >> > + >> >> The indentation and parenthesization is not quite right here: >> ((double) c[i]->min[(idx_count - 1) % >> MIN_AVG_LEN]) >> * 1000 / COVERAGE_RUN_INTERVAL, >> It should be: >> ((double) c[i]->min[(idx_count - 1) % >> MIN_AVG_LEN] >> * 1000 / COVERAGE_RUN_INTERVAL), >> (I think that CodingStyle implies this--I intended that it should--but >> it might require a close reading.) >> >> Also I might just use 1000.0 instead of a cast to double, because I am >> not fond of casts: >> (c[i]->min[(idx_count - 1) % MIN_AVG_LEN] >> * 1000.0 / COVERAGE_RUN_INTERVAL), >> >> > > Thanks for pointing these out. I'll fix accordingly. > > > >> When I can, I like to use build assertions to enforce build-time >> invariants in place of or in addition to comments. (This is level 9 >> on Rusty Russell's "Hard To Misuse Positive Score List" at >> http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html. Everyone >> should read this.) >> >> In this case, I think that you can use BUILD_ASSERT_DECL. >> > > Thanks for sharing this reference! >
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
