[vsipl++] Matlab IO

[Stefan Seefeld]

Assem,

this looks good. I have some (mostly) stylistic comments:

Assem Salama wrote:

> Index: matlabformatter.hpp

For long composite names I'd find 'matlab_formatter.hpp' more readable.
Also, as there will be a binary matlab format, I think a more descriptive
name for this formatter would be 'Matlab_ascii_formatter'.

> ===================================================================
> RCS file: matlabformatter.hpp
> diff -N matlabformatter.hpp
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ matlabformatter.hpp	19 May 2006 15:59:06 -0000
> @@ -0,0 +1,103 @@
> +#ifndef VSIP_CSL_MATLABFORMATTER_HPP
> +#define VSIP_CSL_MATLABFORMATTER_HPP
> +
> +#include <string>
> +#include <vsip/support.hpp>
> +
> +/* Declare our classes that we will use for formatting stream output. Note that
> + * these classes will only work for ascii streams
> + */
> +namespace vsip_csl
> +{
> +
> +  //template <template <typename,typename> class ViewT>
> +  template <typename ViewT>
> +  class MatlabFormatter

Same argument here: Isn't this an 'Matlab_ascii_formatter' ? (Note our naming
conventions, i.e. underscore instead of CamelCase.)

> +  {
> +    /* Constructors */
> +    public:
> +      MatlabFormatter(ViewT v) : v_(v), view_name_("a")  {}
> +      MatlabFormatter(ViewT v,std::string name) 
> +        : v_(v), view_name_(name)  {}
> +
> +
> +      MatlabFormatter() {}

I think only providing the constructor taking two arguments is fine.


> +
> +      ~MatlabFormatter() {}
> +
> +    /* Accessors */
> +    public:
> +      ViewT get_view() { return v_; }
> +      std::string get_name() { return view_name_; }

As your class is really just a placeholder, why don't you make it a
struct with only two public members, and a constructor for convenience ?
There is no need to protect these members by accessor methods...

template <typename ViewT>
struct Matlab_text_formatter
{
    Matlab_text_formatter(ViewT v, std::string const &n) : view(v), name(n) {}
    ViewT view;
    std::string name;
};

> +template <typename T,
> +          typename Block0>
> +inline
> +std::ostream&
> +operator<<(
> +  std::ostream&		                          out,
> +  MatlabFormatter<vsip::Matrix<T,Block0> >        mf)
> +  VSIP_NOTHROW

I don't think we want VSIP_NOTHROW here. If you really do mean to assert
that no exception is thrown from within this operator, you should wrap
the whole body in a try block. But I'm not sure why this would be needed.

Also, I think the second argument should be
'MatlabFormatter<const_Matrix<T,Block0> > const &' (note the two consts).
Similarly for the other dimensions.

Do you have an environment to test the generated format ? We should make
sure that matlab actually accepts it.


Regards,
		Stefan