Code Review Triage
From Audacity Wiki
|This page is associated with the code review initiative. The intention is to classify different issues found in code review, with discussion on the talk page.
Peter 23Mar15: ToDo-D this page is moribund, it has not been used for over four years. If it is no longer considered of use then I suggest that we delete it.
- Gale 23Mar15: AFAIK James used to be keen on this. This is developer rather than QA material. I don't know if the "FIX-ME" and "todo" could be scripted to output to this page or to somewhere else, or if Bugzilla should track these, so that the entry in the code pointed to Bugzilla. I don't think it's clear yet we should delete this page.
- James 24Mar15: We should actually capture the details of the code reviews before Google Code closes down, possibly actually in the code, as they are potentially useful in future. The issues haven't 'gone away'.
We're currently using this triage list for general points that are not specific to one piece of code or algorithm.
- There is a lot of use of 'new/delete' in working with sample buffers and envelope point buffers. These are typically allocated and deallocated with every repaint of the screen. This is inefficient, causes memory fragmentation and is 'unchecked'. The success/failure of the new/delete is unchecked (minor). The indexing into the buffers is unchecked (major). We need a "SmartBuffer" that can as an option allocate on-stack to rectify this. It could also alert us if very large buffer sizes were requested e.g. with zoomed out views of large projects.
- Some files have missing or incomplete header comments. As a result the classes they contain are not being described in Doxygen listings, or indeed in the code.
May Do Now
- The vim indent setting footers at the end of every page are (arguably) just an annoyance. Needs checking on audacity-devel that there is a consensus that they are not such a good idea after all.
- Lots of repetitive code where idioms like
value=constrain(value,min,max);can make the code shorter and clearer. In many cases small helper functions would be good.
- There are lots of cases of long functions where splitting the functions up into component functions each with a well chosen name would be helpful - or at the very least we should add a comment.
- Cut-and-paste code in related classes, not sufficiently exploiting the commonality in classes by using functions in the base class.
- Tag handling is verbose code, rather like how dialog building code was before ShuttleGui. A similar technique could make this code much more readable.
These are comments in the code (as of 9th April 2011) that say 'FIX-ME'. Each of these should be visited, reviewed and cleaned up.
|AudacityApp.cpp(706):||MRUOpen(name); // FIX-ME: Check the return result?||In disabled code.|
|AudacityApp.cpp(761):||// That itself may be a FIX-ME.||OpenFile should return a status, and that status should be checked.|
|BatchCommands.cpp(437):||// FIX-ME: No error reporting on write file failure in batch mode.||Only affects batch mode.|
|BatchCommands.cpp(522):||//FIX-ME: for later versions may want to not select-all in batch mode.||But it is OK at the moment.|
|DirManager.cpp(843):||// FIX-ME: Might we get here without midkey having been set?||Indeed. That would be worrying. This function rebalances directory trees and needs very close scrutiny. Possibly it's behind Bug 137.|
|Menus.cpp(1334):||// FIX-ME: So we have a memory leak of menu items under linux? Oops.||It's not clear why windows needs to delete these menus and linux does not.|
|Project.cpp(2268):||// FIX-ME? This should return a result that is checked.||OpenFile should return a status.|
|Project.cpp(2355):||//FIX-ME: //v Surely we could be smarter about this,||Coding style poor, but not apparently erroneous.|
|Resample.cpp(183):||// FIX-ME: Audacity will hang after this if branch.||Serious, but only active if USE_LIBSAMPLERATE|
|TrackPanel.cpp(4813):||// FIX-ME: Disable this and return true when CutLines aren't showing?||Performance - mouse moves would be quicker with this fix.|
|TrackPanel.cpp(5175):||//FIX-ME: Not necessarily. Haven't checked Track::Note (#if defined(USE_MIDI)).||MIDI and multi-tool mode probably don't get along because of this.|
|WaveTrack.cpp(2118):||// FIX-ME: The track is now in an inconsistent state...||Looks serious, if using multiple sample rates.|
|effects\NoiseRemoval.cpp(193):||// FIX-ME: Should we check return value on Write?||Cleanspeech profile does not flag error if can't write file.|
|export\ExportFFmpegDialogs.cpp(430):||// FIX-ME: Catch XMLFileWriterException||Writing presets does not check for file error.|
|export\ExportFFmpegDialogs.cpp(447):||// FIX-ME: Catch XMLFileWriterException||Reading presets does not check for file error.|
|widgets\Ruler.cpp(692):||// FIX-ME: We don't draw a tick if of end of our label arrays||Very minor.|
|widgets\Ruler.cpp(752):||// FIX-ME: we shouldn't even get here if strPos < 0.||Minor. Worst outcome if there is a problem would be missing ticks.|
|widgets\Ruler.cpp(804):||// FIX-ME: We don't draw a tick if of end of our label arrays||Cut and pasted code.|
|widgets\Ruler.cpp(866):||// FIX-ME: we shouldn't even get here if strPos < 0.||Cut and pasted code.|
|widgets\Ruler.cpp(981):||// FIX-ME: Surely we do not need to allocate storage for the labels?||Custom ruler annotation looks mildly expensive in terms of mallocs / frees.|
Links to some reviews
- TimeTracks.cpp (James)
- AutoRecovery.cpp (James)
- WaveClip.cpp (James,Michael)
- UndoManager.cpp (James)
- ImportFFmpeg.cpp (James)
- FFmpeg.cpp (James)
- TruncSilence.cpp (James)
- Timetracks/Play-at-speed (started)
- ASSERTs. Some should be generating end-user warnings
- Load on-demand
- Label Linking
- ProgressDialog code
- Checking of new/delete malloc/free failure conditions
- Review of TagHandling. There is more processing happening in these functions than at first meets the eye and some is very relevant to stability. I (James) am tending to skip these in quick per-file code reviews because of their verbosity.