Messages in this thread | | | From | KOSAKI Motohiro <> | Subject | Re: [PATCH v3] Add generic exponentially weighted moving average (EWMA) function | Date | Fri, 22 Oct 2010 10:11:38 +0900 (JST) |
| |
> > and nit, I don't understand what you intend by 'const unsigned int'. I > > think we can remove this const safely. it's more easy readable. > > O.K. Will resend. > > Apart from that, what didn't work as you expected?
It works. I meant this const has no meaning.
few additional reviewing comments is here.
> +struct ewma { > + unsigned int internal; > + unsigned int factor; > + unsigned int weight; > +};
I think unsigned long is better because long is natual register size on both 32bit and 64bit arch. and, example, almost linux resource limit is using long or ulong. then uint may have overflow risk if we are using this one on 64bit arch. Does uint has any benefit? (note: scheduler loadavg has already used ulong)
> +struct ewma* > +ewma_add(struct ewma *avg, const unsigned int val) > +{ > + avg->internal = avg->internal ? > + (((avg->internal * (avg->weight - 1)) + > + (val * avg->factor)) / avg->weight) : > + (val * avg->factor); > + return avg;
Hm, if ewma_add has this function prototype, we almost always need to typing "new = ewma_get(ewma_add(&ewma, val))". Is this intentional? if so, why?
Why can't we simple do following?
unsigned long ewma_add(struct ewma *avg, const unsigned int val) { (snip) return ewma_get(avg); }
| |