[vsipl++] Matlab IO

[Jules Bergmann]

Assem, I have 8 comments below.  -- Jules

 > +  template <vsip::dimension_type Dim>
 > +  struct view_header
 > +  {
 > +    data_element header;
 > +    data_element array_flags_header;
 > +    uint32_t array_flags[2];
 > +    data_element dim_header;
 > +    uint32_t dim[Dim + Dim%2]; //the dim has to be aligned to an 8 
byte boundary

[1] Can you make sure this comment fits into 80-columns?

 > +    data_element array_name_header;
 > +  };

 > +  // the read function for real or complex depending of the view 
that was
 > +  // passed in
 > +  template <typename T1,
 > +	    typename ViewT>
 > +  void read(std::istream& is,ViewT v,bool swap_bytes)
 > +  {
 > +    vsip::dimension_type const View_dim = ViewT::dim;
 > +    vsip::Index<View_dim> my_index;
 > +    vsip::impl::Length<View_dim> v_extent = extent(v);
 > +    T1 data;
 > +    typedef typename ViewT::value_type scalar_type;
 > +
 > +    // get num_points
 > +    vsip::length_type num_points = v.size();
 > +
 > +    // read all the points
 > +    for(vsip::index_type i=0;i<num_points;i++) {
 > +      is.read(reinterpret_cast<char*>(&data),sizeof(data));
 > +      swap<T1>(&data,swap_bytes);

[2] The intent of the wrapper template function 'swap' is to let the
compiler infer template parameters automatically.  Since 'data' is of
type 'T1', you can just say:

	swap(&data, swap_bytes);

Unless the inferred type would be incorrect, can you remove the explicit 
parameter?


 > +
 > +// operator to read view from matlab file
 > +template <typename T,
 > +          typename Block0,
 > +	  template <typename,typename> class View>
 > +inline
 > +std::istream&
 > +operator>>(
 > +  std::istream&                                       is,
 > +  Matlab_bin_formatter<View<T,Block0> >               mbf)
 > +{
 > +  matlab::data_element temp_data_element;
 > +  matlab::view_header<vsip::impl::Dim_of_view<View>::dim> m_view;
 > +  typedef typename vsip::impl::Scalar_of<T>::type scalar_type;
 > +  typedef matlab::Subview_helper<View<T,Block0> > subview;
 > +  typedef typename subview::realview_type r_v;
 > +  typedef typename subview::imagview_type i_v;
 > +  vsip::dimension_type v_dim = vsip::impl::Dim_of_view<View>::dim;
 > +
 > +
 > +  // read header
 > +  is.read(reinterpret_cast<char*>(&m_view),sizeof(m_view));
 > +

[3] When does mbf.header.endian get initialized?  Have you written a
unit test for these routines?  (If not, please do that next).


 > +  // do we need to swap fields?
 > +  matlab::swap_header(m_view,mbf.header.endian);

[4] The second arg to swap_header is a bool indicating if bytes should
be swapped.  Passing mbf.header.endian (a uint16_t) will nearly always
be true (unless the matlab file is corrupted).  Can you do something
like:

	bool swap_bytes;

	if      (mbf.header.endian == 'M' << 8 | 'I') swap_bytes = false;
	else if (mbf.header.endian == 'I' << 8 | 'M') swap_bytes = true;
	else
	  VSIP_IMPL_THROW(std::runtime_error(
	    "Matlab file header has invalid endian field"));

	matlab::swap_header(m_view, swap_bytes);


 > +
 > +  // is this complex?
 > +  if(vsip::impl::Is_complex<T>::value && 
!(m_view.array_flags[0]&(1<<11)))
 > +    VSIP_IMPL_THROW(std::runtime_error(
 > +      "Trying to read complex matrix into a real matrix"));

[5] Please change this to "Trying to read a complex view into a real view"

 > +
 > +
 > +  // is this the same class?
 > +  if(!((m_view.array_flags[0] & 0xff) ==
 > +            (matlab::Matlab_header_traits<sizeof(scalar_type),
 > +                  std::numeric_limits<scalar_type>::is_signed,
 > + 
std::numeric_limits<scalar_type>::is_integer>::class_type)
 > +	    ))
 > +    VSIP_IMPL_THROW(std::runtime_error(
 > +      "Trying to read a matrix of a different class"));

[6] Please change this to "Trying to read a view with different class of 
value type"

 > +
 > +  // do dimensions agree?
 > +  if(v_dim == 1) m_view.dim_header.size -= 4; // special case for 
vectors
 > +  if(v_dim != (m_view.dim_header.size/4))
 > +    VSIP_IMPL_THROW(std::runtime_error(
 > +      "Trying to read a matrix of different dimensions"));

[7] Please change this to "Trying to read a view with different dimension"

 > +
 > +  for(vsip::dimension_type i=0;i<v_dim;i++)
 > +    if(mbf.view.size(i) != m_view.dim[i])
 > +      VSIP_IMPL_THROW(std::runtime_error(
 > +        "Matrix dimensions don't agree"));

[8] Please change this to "View dimensions don't agree"


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