[vsipl++] Matlab IO

[Jules Bergmann]

Assem Salama wrote:
 > Everyone,
 >  Patch with Jule's comments.

Assem, this looks good.  I have 4 comments below.  Once you address 
those, please check it in.

				thanks,
				-- Jules

 > ------------------------------------------------------------------------
 > Index: src/vsip_csl/matlab_file.hpp
 > ===================================================================
 > --- src/vsip_csl/matlab_file.hpp	(revision 0)
 > +++ src/vsip_csl/matlab_file.hpp	(revision 0)
 > @@ -0,0 +1,248 @@
 > +/* Copyright (c) 2005, 2006 by CodeSourcery.  All rights reserved. */
 > +
 > +/** @file    vsip_csl/matlab_file.hpp
 > +    @author  Assem Salama
 > +    @date    2006-06-21
 > +    @brief   VSIPL++ CodeSourcery Library: Matlab file class that 
handles
 > +             Matlab files using an iterator.
 > +*/
 > +
 > +#ifndef VSIP_CSL_MATLAB_FILE_HPP
 > +#define VSIP_CSL_MATLAB_FILE_HPP
 > +
 > +#include <iostream>
 > +#include <fstream>
 > +#include <vsip/impl/noncopyable.hpp>
 > +#include <vsip_csl/matlab_bin_formatter.hpp>
 > +
 > +namespace vsip_csl
 > +{
 > +
 > +class Matlab_file : vsip::impl::Non_copyable
 > +{
 > +  public:
 > +    // Constructors
 > +    Matlab_file(std::string fname);
 > +
 > +  // classes
 > +  public:
 > +    class iterator
 > +    {


[1] To be defensive, you should make the following constructor private:

 > +      public:
 > +        iterator(bool end_iterator,Matlab_file *mf) :
 > +	  mf_(mf),
 > +	  end_iterator_(end_iterator) {}

That way only Matlab_file can use it (since it is a friend class).
We know Matlab_file will always pass a valid Matlab_file*, so there
is no need to assert(mf != NULL).

 > +
 > +      public:
 > +	// copy constructors
 > +	iterator(iterator const &obj) :
 > +          mf_(obj.mf_), end_iterator_(obj.end_iterator_) {}
 > +
 > +        // = operator
 > +	iterator&
 > +	operator=(iterator &src)

[2] Since this doesn't modify src, you should make parameter a const:

	operator=(iterator const& src)

This allows the compiler to use the assignment operator under more
conditions (such as assigning from a temporary object).

 > +	{
 > +	  this->mf_           = src.mf_;
 > +	  this->end_iterator_ = src.end_iterator_;
 > +	  return *this;
 > +	}


 > Index: tests/matlab_iterator_test.cpp

[3] Please rename this file to 'tests/matlab_iterator.cpp'

 > ===================================================================
 > --- tests/matlab_iterator_test.cpp	(revision 0)
 > +++ tests/matlab_iterator_test.cpp	(revision 0)
 > @@ -0,0 +1,191 @@
 > +/* Copyright (c) 2006 by CodeSourcery, LLC.  All rights reserved. */
 > +
 > +/** @file    tests/matlab_bin_file_test.cpp
 > +    @author  Assem Salama
 > +    @date    2006-07-18
 > +    @brief   VSIPL++ Library: Test for reading and writing Matlab 
.mat files
 > +             using iterators
 > +*/
 > +
 > +/***********************************************************************
 > +  Included Files
 > +***********************************************************************/
 > +
 > +#include <iostream>
 > +
 > +#include <vsip/support.hpp>
 > +#include <vsip/matrix.hpp>
 > +#include <vsip/tensor.hpp>
 > +#include <vsip_csl/matlab_file.hpp>
 > +#include <vsip_csl/output.hpp>
 > +
 > +#include "test.hpp"

[4] Rembemer to do an 'svn update' before commiting this.  test.hpp is
no longer in the tests directory.



-- 
Jules Bergmann
CodeSourcery
jules at codesourcery.com
(650) 331-3385 x705