[vsipl++] Matlab IO Patch

[Assem Salama]

The reason that I have the comment is because I was planning on reading 
the array name into view_name_. I didn't know how to do that at the time 
so I just read it into a temporary array and commented out the part that 
didn't work. By leaving the comment in there for now, I will not forget 
that I was planning on fixing that part.

Thanks,
Assem Salama

Mark Mitchell wrote:
> 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.
>
>