[vsipl++] Matlab IO

[Assem Salama]

Jules Bergmann wrote:
> Assem Salama wrote:
>> Assem Salama wrote:
>>> Everyone,
>>>  Fixed endian stuff.
>
> Assem,
>
>>> +
>>> +  template <typename T,size_t type_size,bool to_swap_or_not_to_swap>
>>> +  struct swap_value +  { +    static void swap(T *d) {d=d;} +  };
>
> For our coding convention, struct and class names should begin with a 
> capital letter: 'Swap_value'
>
>>> +
>>> +  template <typename T>
>>> +  struct swap_value<T,2,true>
>>> +  {
>>> +    static void swap(T* d)
>>> +    {
>>> +      char *p = reinterpret_cast<char*>(d);
>>> +      std::swap(p[0],p[1]);
>>> +    }
>>> +  };
>>> +
>>> +  template <typename T>
>>> +  struct swap_value<T,4,true>
>>> +  {
>>> +    static void swap(T* d)
>>> +    {
>>> +      char *p = reinterpret_cast<char*>(d);
>>> +      std::swap(p[0],p[3]);
>>> +      std::swap(p[1],p[2]);
>>> +    }
>>> +  };
>>> +
>>> +  template <typename T>
>>> +  struct swap_value<T,8,true>
>>> +  {
>>> +    static void swap(T* d)
>>> +    {
>>> +      char *p = reinterpret_cast<char*>(d);
>>> +      std::swap(p[0],p[7]);
>>> +      std::swap(p[1],p[6]);
>>> +      std::swap(p[2],p[5]);
>>> +      std::swap(p[3],p[4]);
>>> +    }
>>> +  };
>
> You can wrap the 'Swap_value' class with a function:
>
> template <typename T>
> void
> swap_value(T& value, bool swap)
> {
>   if (swap)
>     swap_value<T, sizeof(T), true>::swap(&value);
> }
>
The reason that I didn't want this to be a function is because if I call 
this in a loop, it will execute a condition every iteration of the loop. 
If it is a template parameter, it might be a little better. What do you 
think?
>>> +
>>> +  // swaps an array of values based on a template param
>>> +  template <typename T>
>>> +  void swap_array(T *d, vsip::impl::Int_type<1>)
>>> +  { swap_value<T,sizeof(T),true>::swap(&(d[0])); }
>>> +  template <typename T>
>>> +  void swap_array(T *d, vsip::impl::Int_type<2>)
>>> +  { swap_value<T,sizeof(T),true>::swap(&(d[0]));
>>> +    swap_value<T,sizeof(T),true>::swap(&(d[1])); }
>>> +  template <typename T>
>>> +  void swap_array(T *d, vsip::impl::Int_type<3>)
>>> +  { swap_value<T,sizeof(T),true>::swap(&(d[0]));
>>> +    swap_value<T,sizeof(T),true>::swap(&(d[1]));
>>> +    swap_value<T,sizeof(T),true>::swap(&(d[2])); }
>
> Is there a reason that swap_array takes the array size as a template 
> parameter?
yes, because I call this function when Dim is a template parameter and I 
cannot iterate over this number.
>
> The pros of passing the size this way are primarily in performance.  
> The above code will force the compiler to inline the individual calls 
> instead of performing a loop.
>
> However, the cons are increased code size (both source code and object 
> code), longer compile times, and limiting the situations where the 
> function can be called.
>
> Can you change swap_array to take the array size as a regular parameter.
>
>>> +
>>> +  // swaps the header of a view
>>> +  template <vsip::dimension_type dim>
>>> +  void swap_header(view_header<dim> &header, uint16_t endian)
>
> Instead of passing 'endian' to each function and having it decode 'MI' 
> vs 'IM' to determine if swapping is necessary, it would be better to 
> have operator>> perform the check and pass a boolean parameter:
>
>     void swap_header(..., bool swap_bytes)
>
> This centralizes the knowledge of how matlab files encode endianness 
> into one place in the code instead of replicating it.  It also makes 
> the function's behavior easier to understand.
Sounds much better.
>
>>> +  {
>
> With the 'swap_value' helper function and the 'swap_bytes' bool 
> parameter, these become:
>
>     swap_value(header.header.type, swap_bytes);
>     swap_value(header.header.size, swap_bytes);
>     ...
>
>>> +    if(endian == ('I' << 8 | 'M') )
>>> +    {
>>> +      // swap all fields
>>> +      swap_value<int32_t,4,true>::swap(&(header.header.type));
>>> +      swap_value<uint32_t,4,true>::swap(&(header.header.size));
>>> +      
>>> swap_value<int32_t,4,true>::swap(&(header.array_flags_header.type));
>>> +      
>>> swap_value<uint32_t,4,true>::swap(&(header.array_flags_header.size));
>>> +      swap_value<int32_t,4,true>::swap(&(header.dim_header.type));
>>> +      swap_value<uint32_t,4,true>::swap(&(header.dim_header.size));
>>> +      
>>> swap_value<int32_t,4,true>::swap(&(header.array_name_header.type));
>>> +      
>>> swap_value<uint32_t,4,true>::swap(&(header.array_name_header.size));
>>> +      swap_array<uint32_t>(header.dim, vsip::impl::Int_type<dim>());
>>> +      swap_array<uint32_t>(header.array_flags, 
>>> vsip::impl::Int_type<2>());
>>> +    }
>>> +  }
>>> +
>>> +  // generic reader that allows us to read a generic type and cast 
>>> to another
>>> +  +  // the read function for real or complex depending of the view 
>>> that was
>>> +  // passed in
>
> What does "generic type" mean in the above comment?
>
> A comment like the following would be more clear:
>
> // Read a N-dimensional matlab XXX.  The value type of the file object
> // is specified by T1 and can be different from the value type of the
> // VSIPL++ view.  For views of complex data, this function is called
> // once for the real subview and once for the imaginary subview.
>
>>> +  template <typename T1,
>>> +        typename ViewT>
>>> +  void read(std::istream& is,ViewT v,uint16_t endian)
>>> +  {
>>> +    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;
>>> +    typedef void (*fn_type)(T1 *data);
>>> +    fn_type swap_fn;
>>> +
>>> +    // get num_points
>>> +    vsip::length_type num_points = v.size();
>>> +
>>> +    // figure out if we need to do endian swaps
>>> +    if(endian != ('M' << 8 | 'I'))
>>> +      swap_fn = swap_value<T1,sizeof(T1),true>::swap;
>>> +    else
>>> +      swap_fn = swap_value<T1,sizeof(T1),false>::swap;
>>> +
>>> +    // read all the points
>>> +    for(vsip::index_type i=0;i<num_points;i++) {
>>> +      is.read(reinterpret_cast<char*>(&data),sizeof(data));
>>> +      swap_fn(&data);
>
> With 'swap_value' wrapper, this becomes:
>
>       swap_value(data, swap_bytes);

>
>>> +      put(v,my_index,scalar_type(data));
>>> +
>>> +      // increment index
>>> +      my_index = vsip::impl::next(v_extent,my_index);
>>> +    }
>>> +
>>> +  }
>>> +
>
>
>>> +
>>> +// 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));
>
> After reading the header, this is the place to check 'endian' to 
> determine if byte swapping is necessary:
>
>     bool swap_bytes = (endian != 'M' << 8 | 'I');
>
>>> +
>>> +  // do we need to swap fields?
>
> Since we already know if it is necessary to swap bytes, the comment 
> might be:
>
>     // Swap header fields if necessary.
>
>>> +  matlab::swap_header(m_view,mbf.header.endian);
>>> +
>>> +  // 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"));
>>> +
>>> +
>>> +  // 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"));
>
> The error message should mention "view" instead of "matrix":
>
>     "Trying to read a view 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(std::runtime_error(
>>> +      "Trying to read a matrix of different dimensions"));
>
>     "Trying to read a view with different dimensions"
>
>>> +
>>> +  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"));
>
>     "View dimensions don't agree"
>
>
>