[vsipl++] Matlab IO

[Jules Bergmann]

Assem Salama wrote:
 > Everyone,
 >  This patch adds a test to the test directory to test low level Matlab
 > IO interface.

Assem,

This looks good, I have 5 comments below, once those are addressed,
please check this in. -- Jules

 > ------------------------------------------------------------------------
 >
 > Index: tests/matlab_bin_file_test.cpp
 > ===================================================================
 > --- tests/matlab_bin_file_test.cpp	(revision 0)
 > +++ tests/matlab_bin_file_test.cpp	(revision 0)

[1] Please add the standard header: copyright, file, author, date,
and brief description.


 > +template <typename T>
 > +void tensor_test(int m, int n, int o, std::ofstream &ofs, char *name)
 > +{

[2] instead of representing view dimensions as 'int', can you use
'length_type' instead?  I expected this type of conversion to be
caught by GCC's '-W -Wall' but apparently not.

 > +  Tensor<T> a(m,n,o);
 > +  T         value;
 > +
 > +  value = 0;
 > +  for(int i=0;i<m;i++) {
 > +    for(int j=0;j<n;j++) {
 > +      for(int k=0;k<o;k++) {
 > +        value = increment(value);
 > +        a.put(i,j,k,value);

[3] Likewise, instead of using 'int' to iterate over indices, can you
use 'index_type'?

 > +      }
 > +    }
 > +  }
 > +
 > +  // write it out to file
 > +  ofs << Matlab_bin_formatter<Tensor<T> >(a,name);
 > +}


 > +template <typename T>
 > +void vector_input_test(int m, std::ifstream &ifs, char 
*name,Matlab_bin_hdr &h)
 > +{
 > +  Vector<T> a(m);
 > +  T         value,input_value;
 > +
 > +  ifs >> Matlab_bin_formatter<Vector<T> >(a,name,h);
 > +
 > +  value = 0;
 > +  for(int i=0;i<m;i++) {
 > +    value = increment(value);
 > +    input_value = a.get(i);
 > +    assert(value == input_value);

[4] Inside of tests, you should use 'test_assert()' instead of 
'assert()'. This way, when you configure the library to run fast (which 
usually means turning off assertions with -DNDEBUG), the tests will 
still check correctness.

 > +  }
 > +#if DEBUG == 1
 > +  cout << a << endl;
 > +#endif
 > +


 > Index: src/vsip/impl/layout.hpp
 > ===================================================================
 > --- src/vsip/impl/layout.hpp	(revision 144405)
 > +++ src/vsip/impl/layout.hpp	(working copy)
 > @@ -1089,6 +1089,12 @@
 >
 >    static type offset(type ptr, stride_type stride)
 >    { return ptr + stride; }
 > +
 > +  static T* get_real_ptr(type ptr)
 > +    { return ptr; }
 > +  static T* get_imag_ptr(type ptr)
 > +    { return ptr; }
 > +

[5] I would think that when calling calling get_imag_ptr on a real 
pointer, it should either return NULL or throw an exception.  To return 
a pointer to the real data seems wrong.

The idea behind defining this function is to allow code to compile that
we do not actually expect to execute at runtime, i.e.:

	value_type* ptr;

	if (Is_complex<value_type>::value)
	{
	  ...
	  get_imag_ptr(ptr);
	  ...
	}
	else // must be real
	{
	  ... code that doesn't use get_imag_ptr() because ptr refers
	  ... to real data.  Trying to use get_imag_ptr() is a design
	  ... error.
	}

For now, lets throw a std::runtime exception "Accessing imaginary part
of non-complex pointer".

 >  };
 >
 >
 > @@ -1147,6 +1153,12 @@
 >
 >    static type offset(type ptr, stride_type stride)
 >    { return type(ptr.first + stride, ptr.second + stride); }
 > +
 > +  static T* get_real_ptr(type ptr)
 > +    { return ptr.first; }
 > +  static T* get_imag_ptr(type ptr)
 > +    { return ptr.second; }
 > +
 >  };


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