Difference between revisions of "Code Review Triage"

From Audacity Wiki
Jump to: navigation, search
(New review done.)
(Comment on tag handlers and ASSERTs)
Line 27: Line 27:
 
* [http://code.google.com/p/audacity/source/browse/audacity-src/trunk/src/TimeTrack.cpp?spec=svn10205&r=10189#66 TimeTracks.cpp (James)]
 
* [http://code.google.com/p/audacity/source/browse/audacity-src/trunk/src/TimeTrack.cpp?spec=svn10205&r=10189#66 TimeTracks.cpp (James)]
 
* [http://code.google.com/p/audacity/source/browse/audacity-src/trunk/src/AutoRecovery.cpp?spec=svn10205&r=10189#168 AutoRecovery.cpp (James)]
 
* [http://code.google.com/p/audacity/source/browse/audacity-src/trunk/src/AutoRecovery.cpp?spec=svn10205&r=10189#168 AutoRecovery.cpp (James)]
 +
  
 
==Pending Reviews==
 
==Pending Reviews==
Line 32: Line 33:
 
* Timetracks/Play-at-speed ''(started)''
 
* Timetracks/Play-at-speed ''(started)''
 
* Multi-Clip
 
* Multi-Clip
* ASSERTs
+
* ASSERTs. ''Some should be generating end-user warnings''
 
* Load on-demand
 
* Load on-demand
 
* Label Linking
 
* Label Linking
 
* ProgressDialog code
 
* ProgressDialog code
 
* Checking of new/delete malloc/free failure conditions
 
* 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.''

Revision as of 12:51, 31 January 2010

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.




We're currently using this triage list for general points that are not specific to one piece of code or algorithm.


Urgent

  • 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.


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.


Do Later

  • Tag handling is verbose code, rather like how dialog building code was before ShuttleGui. A similar technique could make this code much more readable.


Links to some reviews


Pending Reviews

  • Timetracks/Play-at-speed (started)
  • Multi-Clip
  • 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.