ItemFileDescriptionConsensusWhoStatusNotes
1ControlExec.cpperror checking: Return of pointers from various methods (e.g. GetParamsInstance(), GetVisualizer() not checked for NULL in numerous methods, resulting in abort. No error logged where return value is checked. Add error handling or asserts as appropriate()y openThis is still not done. Numerous instances where methods that return pointers are called and the return value is not checked against NULL.
2ControlExec.cppDocumentation for ActivateRender() unclear. What happens when parameter 'instance' is not known? Where does 'instance' come from? What are possible values for parameter 'type'? Make use of doxygen \sa (see also)y Done 
3ControlExec.cppShouldn't ControlExec be derived from MyBase to make use of error reporting?y Done 
3aControlExec.cppMany methods return error conditions, but don't log error messages. E.g. SetCurrentParamsInstance(), ActivateRender()y Done 
4ControlExec.hDocumentation for RemoveParams() implies that all instance identifiers will become invalid after calling this method. This appears to have the potential for nasty side effects. Why not simply keep instance params unique and not force them to be contiguous.y DoneSuch a change to instance identifiers would require lots of code changes.  Instead, since the identifiers are only manipulated by the InstanceParams, the appropriate ParamsMgr methods were made private and the InstanceParams made a friend of ParamsMgr.
5ControlExec.hParams management interface seems unnecessarily complex. Hopefully can be simplified or more extensively documented. There are over 20 methods for Params access. Not clear when to use what.y closedNeeds further review and discussion. See example code. Params management methods have been moved to ParamsMgr class.
6ControlExec.cppMemory leak in ControlExec::LoadData(). _dataStatus not freed if already allocated.y Fixed

an:_dataStatus is deleted in destructor

jc: There is nothing to prevent LoadData() from being called twice on same instance.

7ControlExec.cppHardcoded window with and height in ControlExec::NewVisualizer()?y DoneRemoved.  width and height are set in resizeViz.
8ControlExec.cppHow is FindInstanceIndex() used? It takes a RenderParams ptr as a parameter, but there appears to be now way to obtain a RenderParams ptr from the ControlExec  DoneReview params api first.  This is currently only used to perform enable/disable operations on sets of RenderParams, and the instance index is needed for that. 
9ControlExec.cppSetErrorHandler() is a no-op. Can it be deleted?y Done 
10ControlExec.cppValidateParams is a no-op. Can it be deleted?y Done 
11ControlExec.cppUndo and Redo return Params ptrs. Which Params? Can anything useful be done these pointers if the caller doesn't know which Params instances or types that they refer to?  Closed

Need to evaluate further. May not be needed. 

This Params is currently only used to determine whether a render is required, so its type and instance is not needed. If we identify a need for more information during undo/redo then we should modify the API at that time.

12ControlExec.hIs there a reason that the ControlExec is derived from ParsedXml class? Would composition be more appropriate?y Donedo if easy
13ControlExec.cppAre the log file facilities (OpenLogFile(), etc) needed? What are they for?N/A   
14ControlExec.cppShouldn't be a static class (static members and methods) without justification.  Done 
15ParamsExplore making static params class a true class (aka ParamsMgr), a pointer of which can be handed to routers, etc. so that they  don't need access to controlexec.  Done 
16ControlExec.cpp

Resource mgt:

  • ParseMgr allocated from heap and never freed.
  • GetCommandText() allocates string that can never be freed
  • Test for NULL before calling delete (e.g. delete _sessionParser, delete in destroyParams())
y Done 
17ControlExec.cppControlExec should be hidden from renderers, Visualizer, TextObject, etc.. If these classes need access to resources owned by the ControlExec (e.g DataMgr) provide only as much access as needed. I.e. Pass a handle to the DataMgr to the classes that need access.y DoneThis has been removed from all the Render classes except still kept as a private variable in the Renderer class, as the control exec is needed when performing Undo/Redo of renderer enabling.
18ControlExec.hUse 'const' qualifiers for Get accessor methods as appropriate.y Done 
  • No labels