|Subject:||Re: Static code analysis of Octave|
|Date:||Fri, 14 Aug 2015 13:33:28 -0700|
On 08/14/2015 05:05 AM, address@hidden wrote:
It's always good to get an outside analysis of things. However, the code can be very complex which can fool some analyzers. One doesn't try to write complex code, but sometimes one is forced to.
The example in mex.cc is one of those. Octave is required to support the MeX interface from Matlab (a 'C' API supported in C++). The global variable mex_context is used not in the local function, but in other functions that may be invoked through call_mex(). For the life of the call_mex function the global value assumes a certain value. At the end of the function the original global value is restored by an unwind protect frame object and all is well.
The example in tmpdir.c does not belong to Octave. Octave uses gnulib to get access to basic cross-platform functions like tmpdir(). This does look like a problem which you might want to report on the gnulib site. It only affects Windows platforms.
There isn't any problem in colamd.cc. The Pinv pointer is created by the OCTAVE_LOCAL_BUFFER macro which expands out around a trivial class to create a pointer to memory which is reliably cleaned up. The internal class for octave_local_buffer uses C++ new/delete mechanisms so an exception would have been thrown earlier in the code if memory was not available.
The usage of
instance = new XXX
is a relic of bad compilers. However, it's not even as bad as it looks. I count 34 examples of this usage in liboctave and libinterp and these are all for singleton items that are created once when Octave starts, but never again. That's a pretty small overhead.
I agree that something looks wrong with cellfun.cc. It seems like the second is_object branch should be deleted since it is never operative.
The code in kpse.cc is ancient and inherited. I'm not surprised that there was something there. I cleaned up the example you mentioned.
The example in load-save.cc also seems valid so I fixed that.
In oct-stream.cc, I don't see a problem with
while (is && !is.eof() ...
If you look at the C++ istream class you will find that evaluating the stream in a boolean context is the equivalent of (!is.fail && !is.bad) so we already have the equivalent of !is.fail in the while loop conditional.
In Sparse.cc the code looks wrong, but in fact is just complex. Octave stores sparse matrices in a compressed column format and
for (i = i; i < u; i++)is exactly needed to determine how many elements are in this particular column.
I made the changes that looked good on the development branch here (http://hg.savannah.gnu.org/hgweb/octave/rev/610c74748518).
Thanks for reviewing Octave.
|[Prev in Thread]||Current Thread||[Next in Thread]|