Messages in this thread | | | From | Bruno Randolf <> | Subject | Re: [PATCH v3] Add generic exponentially weighted moving average (EWMA) function | Date | Fri, 22 Oct 2010 10:25:30 +0900 |
| |
On Fri October 22 2010 10:11:38 KOSAKI Motohiro wrote: > 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)
You know more about this than me. I have no specific reason to use unsigned int. I'll change it to unsigned long, if that's better.
> > +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); > }
Hmm, I guess that depends on the way you want to use it. In my case, most of the times when I add a value to the average, I don't need to get the value. I'd call ewma_add() many more times than ewma_get(). Having the functions defined like this gives us the flexibility to choose and IMHO ewma_get(ewma_add(&ewma, val)) isn't so bad?
bruno
| |