[eigen] Signed-ness of size types, and better constructors for Matrix

[Patrick Mihelich <patrick.mihelich@xxxxxxxxx>]

Hi,

Maybe it is just me, but it bothers me that the type for sizes and indices throughout Eigen is signed int. This is inconsistent with the standard, which requires size_t and the X::size_type used by STL containers to be unsigned integral types. I prefer that convention, using unsigned for quantities that are non-negative by definition. When I'm iterating over a container (from STL, Boost, whatever) by index, I'm used to writing

for (unsigned i = 0; i < c.size(); ++i)

but when I have an Eigen matrix and do something like

for (unsigned i = 0; i < m.rows(); ++i)

I get a compiler warning for comparing signed/unsigned values. Admittedly this is a minor annoyance, but I think that consistency with the standard is a Good Thing if you want to minimize surprise for the C++ community at large. I'd rather see size_t or unsigned int used for sizes/indices.

Anyway, this got me into trouble in combination with a separate API issue: the constructors for Matrix are not quite coherent. I wrote some code like this:

void foo(unsigned rows, unsigned cols) {
  Eigen::MatrixXf m(rows, cols);
  ...
}

and got a compile error because it's ambiguous whether to use Matrix(int, int), Matrix(const float&, const float&) or Matrix(const double&, const double&). Ouch! Currently the coefficient constructors use static asserts to check if they are appropriate and the (int, int) constructor has code to choose the proper behavior. This approach has a few issues: it doesn't prevent the constructors from conflicting, construction of 2-vectors by general Scalar coefficients (e.g. adolc types) is unsupported, and it unnecessary muddles the purpose of the (int, int) constructor. The Matrix constructors should use some equivalent of boost::enable_if so they are only available when they should be, e.g.:

// (int, int) contructor, enabled for dynamic-sized matrices only
Matrix(int rows, int cols, boost::enable_if_c<SizeAtCompileTime == Dynamic>::type* = 0);

// 2-coefficient constructor, enabled for fixed-size vectors only
Matrix(const Scalar& x, const Scalar& y, boost::enable_if_c<IsVectorAtCompileTime && SizeAtCompileTime == 2>::type* = 0);

// etc. for 3, 4-coefficients
// Matrix(const float&, const float&) and Matrix(const double&, const double&) no longer needed.

Obviously some syntactic sugar is possible, and to avoid cluttering the documentation you could wrap the dummy arguments in a macro like EIGEN_DUMMY_ARG that is (essentially) identity or no-op depending on whether it's parsed by the compiler or Doxygen:

#ifdef EIGEN_PARSED_BY_DOXYGEN
#define EIGEN_DUMMY_ARG(arg)
#else
#define EIGEN_DUMMY_ARG(arg) , arg

Matrix(int rows, int cols EIGEN_DUMMY_ARG(boost::enable_if_c<SizeAtCompileTime == Dynamic>::type* = 0) );
// etc.

enable_if is a trivial application of SFINAE, and all the checks are already being done in static asserts or ifs, so I'd be surprised if this added anything to compile time.

Thanks for all your great work; it's nice when I can find only trivialities like these to complain about.

-Patrick