On Monday 20 July 2009 08:29:03 steve-k@... wrote:
> Hi Tobias,
> > On Saturday 11 July 2009 11:08:12 steve-k@... wrote:
> > > Also, I found the code a bit complicated and slightly
> > > dangerous in conjunction with partially broken compiles,
> > > so I'd suggest to modify a few things which are part
> > > of the enclosed patch.
> > If the compiler is broken it should be fixed. Then they will be glad I
> > found
> > the bug for them :-)
> > Anyway, I added some parentheses to make it more safe. I think it's good
> > having the checks there.
> ok, I understand that the checks are required, but isn't it better to use
> strtoul instead of atoi here and avoid the checks ?
If you specify a high enough value for the configuration variables (>INT_MAX),
the result will turn negative when casted to int. So if strtoul should be
used, the typing should be changed to UL all the way through. However, that
would complicate the code a tiny bit and I don't think it's worth the effort.
And also it depends on how you see the class. As it is written now, the values
can be set from outside the class. Even if that possibility is not used at the
moment I think it is still a good thing to check the arguments so the check
> Also, the compiler problems are not the missing parentheses, but the
> use of the same variable names for function arguments and class
> members. This causes problems in some versions of GCC.
I don't call that dangerous programming. I call it a bug ;-)
The "delay" function argument should of course not be the same as the member
I don't like using non-explaining variable names or function arguments like
"val" unless it's really a general variable used for different types of values.
I prefer changing the names of the member variables instead. The "m_" prefix
solution is not the prettiest one but it's the one I prefer in this case.
I do agree with you that it is a slightly dangerous programming style, which
the bug showed, but not choosing self-explaining variable names can also be a
hassle. Maybe not in these simple functions but in larger functions this
becomes a problem.
> Additionally, I experimented a bit with an ARM9 embedded board
> and had some performance problems due to double precision FP
> in the new SigLevel code. I think single precision FP is sufficient,
> when smaller blocks are used and the sub-results are accumulated
> in a separate step (see enclosed patch).
Hmmm... Yes, maybe there is a way to use single precision instead of double
precision but the code you provided does not do the same thing as the old
code. It calculates the sum of multiple RMS-values, not the RMS value of the
given block. Not the same thing as far as I can see. Don't you agree?
73's de SM0SVX / Tobias
> vy 73s de Steve, DH1DM