[vsipl++] Matlab IO Patch

[Mark Mitchell]

Assem Salama wrote:

> +  // is this the same class?
> +  if(!(m_view.array_flags[0] == 
> +            (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(vsip::impl::unimplemented(
> +      "Trying to read a matrix of a different class"));

> +  // 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(vsip::impl::unimplemented(
> +      "Trying to read a matrix of different dimensions"));

"unimplemented" should only be use for things that we plan to implement,
but haven't.  Do we really ever expect to read a matrix of the wrong
size?  I think most of these things should just be errors, not
unimplemented.

> +    /*
> +    strncpy(mbf.view_name.data(),
> +            reinterpret_cast<char*>(&m_view.array_name_header.size),
> +	    length);
> +    mbf.view_name[length] = 0;
> +    */

No commented-out code.  Assem, I know this has been pointed out before;
please check your patches for this before submission.


> +    // Because we don't know how the data was stored, we need to instantiate
> +    // generic_reader which can read a type and cast into a different one
> +    if(temp_data_element.type == matlab::miINT8) 
> +    {
> +      if(i==0)matlab::read<int8_t,T,r_v>(is,subview::real(mbf.v));
> +      else    matlab::read<int8_t,T,i_v>(is,subview::imag(mbf.v));
> +    }
> +    else if(temp_data_element.type == matlab::miUINT8) 
> +    {
> +      if(i==0)matlab::read<uint8_t,T,r_v>(is,subview::real(mbf.v));
> +      else    matlab::read<uint8_t,T,i_v>(is,subview::imag(mbf.v));
> +    }

This cascase of if's could be a case statement, something like this:

  istream (*fn)(istream&, subview::subview_type);
  switch (temp_data_element.type) {
    case matlab::miINT8:
      fn = matlab::read<int8_t,T,r,v>;
      break;
    case matlab::miUINT8:
      fn = matlab::read<uint8_t,T,r,v>;
      break;
    ...
  }
  if (i == 0)
    fn(is,subview::real(mbf.v));
  else
    fn(is,subview::imag(mbf.v));

I don't know if that's better; just suggesting it as possibly tidier.

> +  /// This struct is just used as a wrapper so that we can overload the
> +  /// << operator
> +  template <typename ViewT>
> +  struct Matlab_text_formatter
> +  {
> +    Matlab_text_formatter(ViewT v) : v_(v), view_name_("a")  {}
> +    Matlab_text_formatter(ViewT v,std::string name) :
> +      v_(v), view_name_(name)  {}
> +
> +    ViewT v_;
> +    std::string view_name_;
> +  };

Another approach, is to add a "write" function to Matlab_text_formatter:

  void
  Matlab_text_formatter::write(ostream& os) {
    // Whatever is currently in operator<<
  }

  inline void
  std::ostream& operator<<(std::ostream& os, Matlab_text_formater mf) {
    mf.write(os);
    return os;
  }

This is somewhat more "object-oriented".  One advantage is that you then
have a useful comment for Matlab_text_formatter:

  // A Matlab_text_formatter writes the contents of a view to a stream,
  // using the Matlab file format.

-- 
Mark Mitchell
CodeSourcery
mark at codesourcery.com
(650) 331-3385 x713