Created attachment 254 [details] Example that fails linking I recently came across a potential problem caused by providing the EIGEN_DEFAULT_TO_ROW_MAJOR define. The problem is when you are using libraries built with different options then there can be problems during linking because MatrixXd will have a different types depending on whether -DEIGEN_DEFAULT_TO_ROW_MAJOR was used or not. This makes it hard to use libraries compiled with different options in the same programs. Because when you include headers with MatrixXd, the type doesn't reflect the type when the library was built. I've attached a minimal example with program c.cpp that uses libraries a.cpp and b.cpp that are compiled with and without -DEIGEN_DEFAULT_TO_ROW_MAJOR. This causes c.cpp failing to compile, because Eigen::MatrixXd isn't the same type in a.o and b.o. If someone runs into this problem there are few options: 1. Re-compile library b.cpp with the -DEIGEN_DEFAULT_TO_ROW_MAJOR 2. Modify the header files for library b to void b(Matrix<double, Dynamic, Dynamic, ColMajor> M); 3. Re-organize my code so that I do not include a.h and b.h in the same compilation unit. Option 1. is probably easiest but sometimes one uses pre-packaged libraries, e.g. Ubuntu packages. Option 2. would be good, if library developers ensured to be explicit on ordering on their public interfaces. By not providing EIGEN_DEFAULT_TO_ROW_MAJOR means that MatrixXd has an explicit ColMajor ordering. I see the usefulness with having the EIGEN_DEFAULT_TO_ROW_MAJOR, but I'm worried that it can lead to incompatibilities. Is there some better way to handle this?
Some options do break ABI compatibility. EIGEN_DEFAULT_TO_ROW_MAJOR is one of them, but not the only one. Another is EIGEN_DONT_ALIGN_STATICALLY, for example. I think that this is a documentation/communication issue. These features are very useful to some people who know what they're doing but clearly they should not be used in code that needs to link with arbitrary other Eigen-using code.
Actually, I never saw a really convincing argument for EIGEN_DEFAULT_TO_ROW_MAJOR. If you need to interface with another library, which uses row-major matrices, I would say you need to write Matrix<double, Dynamic, Dynamic, RowMajor>::Map(...) etc anyways, or you need to assert that EDTRM is defined. Also, whenever you use .data() or Map or initialize (fixed size) matrices using a scalar pointer, you need to explicitly specify the intended order of your matrix, otherwise you get different behavior depending on the compile options, i.e. EDTRM even breaks API compatibility! One single case I can think of, where EDTRM could be useful is if an algorithm happens to be faster/better vectorizable in row-major mode (and the implementor is to lazy to add that option each time, or just wants to quickly check if it really is faster). I'm afraid it is too late to disallow the macro, though ...
Agreed - we got a bit of a mess with our own library (pointclouds.org) for trying to be too clever and support both row major and column major matrices. As Cristoph mentioned, the fact that EDTRM breaks _API compatibility_ is huge. Basically a core typedef that you were relying on to be something (e.g., MatrixXf) is now something completely different. I would have also liked to see a comparison / performance test of all Eigen operators in column vs row major to see what the differences in performance are.
I must say that I dislike this EIGEN_DEFAULT_TO_ROW_MAJOR. From my point of view this should only be an internal trick to be used by our unit tests.
If everyone agrees that the macro is mostly a hack used for testing, how about deprecating the macro (i.e. let it raise a warning explaining the issue) and replace it by something like EIGEN_DEFAULT_TO_ROW_MAJOR_YES_I_KNOW_THIS_BREAKS_ABI_AND_API_COMPATIBILITY? (I deliberately exaggerated with the length of the new identifier) I think no "normal" user will use the macro anyways, and "pro"-users should be able to adapt their compile flags and insert RowMajor template parameters where required. I admit that unit-tests are one case where this flag /might/ be useful, but as I understand it, this requires unit-testers to compile and run both w/ and w/o the flag? Or does the testing framework do this automatically somehow? Otherwise it might be better to automatically run tests both for Column- and RowMajor (maybe in the same spirit as e.g. test/product_trmm.cpp does it already, i.e. also for all combinations)
This should be decided before releasing 3.1. If it's a WONTFIX it should at least be reconsidered in the far future of Eigen 4.0.
For the moment, I moved the EIGEN_DEFAULT_TO_ROW_MAJOR macro to the developer-only section. I also added a warning that macros can break ABI/API and that that is dangerous. Changeset 08792adb00d6.
It should be clear now that this macro should not be considered by user code. For the existing code, well that's too late already. I'll add an entry for 4.0.
-- GitLab Migration Automatic Message -- This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/422.