Re: [Audacity-quality] Dangerous code... Ctrl_Wheel_Crash

[Al D. <bus...@gm...>]

On Friday 29 January 2010 08:28:27 James Crook wrote:
> *
> 
> 
> From: *http://wiki.audacityteam.org/wiki/Bug:Ctrl_Wheel_Crash
> *
> James: *FindLinearTickSizes() had dangerous code in it that could
>  have been spotted by a code review.
> *
> AWD*: I'm not sure I'd say the dangerous part was in
> FindLinearTickSizes(); it really shouldn't have to deal with
> ViewInfo->zoom being set to zero. I bet a lot of stuff crashes with
>  zoom set to zero, this was just the first one that came up. The
>  formula for zoom ratio was dangerous -- it should have been
>  exponential, was this weird thing with a discontinuity.
> 
> 
> *
> 
> James*
> FindLinearTickSize has a for(;;) loop, and that is a dangerous
> construct.  The test that makes it safe is an ASSERT that only
>  appears in debug versions.  That makes it unsafe in release
>  versions. The formula for zoom ratio was not only dangerous, it
>  was also incorrect.  Just because it is dangerous doesn't mean the
>  code I pointed to isn't dangerous too.
> 

OK, that's fair. If the loop had been written as "while (units >= d)" 
it would have been just as dangerous -- the safe loop test was the 
assertion. Of course, at that point in the code there's nothing to be 
done... we're going to get some really weird results whether it 
crashes or not.

> Both issues could have been found with a code review.
> 
> I'd like to see Audacity team develop an approach to code
>  reviewing. There are reviewing tools in the google code site
>  already set up that we could use.  It could save us from some of
>  these embarrassing errors.
> 

I agree. A lot of the bugs I've come across (and written) would likely 
have been spotted by a second set of eyes.

> Also - THANK YOU for solving this Al.
> 

No prob, 'eh? Thanks for all your work on Bugzilla and the SVN move. 
It's all looking pretty good from what I can see.

 - Al

> --James.
>