Created attachment 389 [details] Patch to resolve the problem == Problem == When using Eigen::AutoDiffScalar<T> as a datatype in an Eigen::Matrix<>, some algorithms like Eigen::SVD do not compile. There are multiple problems - the most difficult one to solve are the calls to min. The situation is somewhat more complicated because Visual C++ defines min as a macro. This enforces Eigen code to disable argument dependant lookup on these calls "(min)(foo,bar)" - which is very ugly. == Solution == The solution to just #undef min is not allowed because it may break user code. Unfortunately Visual C++ (and now also g++) provide #pragma push_macro to same contents of a macro. So the first problem can be resolved by putting the following statement at the top of all Eigen includes: #pragma push_macro("min") #undef min And reverting this at the bottom of all includes: #pragma pop_macro("min") After this it is possible to enable argument dependant lookup on calls to min(). But since Eigen code always does: using std::min; we import a template in the local namespace which then catches all occurences of min(TYPE1,TYPE2); where TYPE1==TYPE2 So if TYPE1==AutoDiffScalar<T> this fails to compile because std::min cannot handle AutoDiffScalar. Even if a specialized function min(AutoDiffScalar<T>, AutoDiffScalar<T>) is provided this fails because there is an ambiguity. The solution provided here is to remove all occurences of using std::min; And provide Eigen::min functions which are not templated. I do not like this solution, but at least it does the job. Probably C++11 could do a better job here but I gather this is not an option.
I honestly was not aware that putting a function in parentheses prevents ADL. However, I would suggest the following solution. // define this in Macros.h: #define EIGEN_NO_MACRO // then use this: { using std::min; X res = min EIGEN_NO_MACRO (a, b); } Without `using std::min` the behavior of min on built-in types would rely on having a min defined elsewhere. Disadvantage: Code is cluttered with stupid macros Advantage: does not rely on non-standard push_macro/pop_macro. As this issue might also concern things in Core with custom types, I changed the component and increased the severity.
As far as I know only Visual C++ defines min/max as macros (talking about lower case here). And this compiler was also the only one which had push_macro/pop_macro (probably not coincidence). GCC adopted that feature later - but we don't need it there. As you mention this is an issue for all custom types. We assume that all custom types have an operator<() defined. We could also use type traits to enable a template through SFINAE. Like this (untested): namespace Eigen { template<typename T> struct Traits { const static bool has_special_compare; } template<typename T> const bool Traits<T>::has_special_compare = false; //for each problematic datatype template<> const bool Traits<special_t>::has_special_compare = true; template<typename T> min( T const &x1, T const &x2, typename boost::disable_if_c<Traits<T>::has_special_compare>::type* = 0) } Probably just without involving boost here. And this is also problematic because the out-of-class specialization cannot be a template so disabling for generic AutoDiffScalar<T> is not possible either.
(In reply to comment #2) > As far as I know only Visual C++ defines min/max as macros Unfortunately, I've seen quite some libraries, which also think they need to define min and max. Of course, we could #error if min or max is defined (after checking for MSVC). > template through SFINAE. Like this (untested): Maybe I'm missing something, but what would that solve what `using std::min;` would not solve? If min is not in parentheses, argument dependent lookup kicks in for custom types, which have their own min implementation. If a type neither has a specialized min implementation, nor operator< defined, we can't do anything anyways.
(In reply to comment #2) > As far as I know only Visual C++ defines min/max as macros The typical culprit seems to be <windows.h>, so e.g. MinGW/Cygwin users with old GCC-versions could also be in trouble. Unless you find a case where the `using std::...` does not work as intended, I'd prefer that over redefining min/max the way you suggested. I slightly prefer preventing macro expansion over #undefining the macros, but I can live with the latter, if this is generally prefered. The question is what to do, if the compiler does not support pushing macros, but min/max are defined. I renamed the bug to represent only the macro issue. If there are other issues regarding SVD with AutoDiff, please open a new bug.
I did not know the parenthesis blocked ADL. I think Christoph's trick is the best solution, though it's quite ugly to write. We could also hide this hack in a numext::min function, and use numext::min throughout Eigen when min is applied to scalars.
(In reply to comment #5) > I did not know the parenthesis blocked ADL. I think Christoph's trick is the > best solution, though it's quite ugly to write. > We could also hide this hack in a numext::min function, and use numext::min > throughout Eigen when min is applied to scalars. You mean something like this: namespace numext { template<class T1, class T2> get_min_max_return_type<T1, T2>::type (min)(const T1& x, const T2& y){ using std::min; return min EIGEN_NO_MACRO(x,y); } } and keep the (min) protection. Then replace all using std::min by using numext::min, or directly write (numext::min)(a,b). That appears possible (though I'm not sure how to implement get_min_max_return_type in a portable way). The easiest solution without cluttering the code seems to be to just #undef offending macros and possibly give a warning about that if #pragma push/pull are not available.
I have users for which #undef is not an option. This is what we were doing initially. To get rid of the parenthesis protection I suggest: namespace numext { template<class T> T minimum(const T& x, const T& y){ using std::min; return min EIGEN_NO_MACRO(x,y); } } and in Eigen code we remove the "using std::min;" and replace " (min)" by " numext::minimum".
Yes, that seems to be sufficient. I somehow thought, we need min/max with different types at some places, but that wouldn't have worked with our current mechanism anyways ...
https://bitbucket.org/eigen/eigen/commits/192873018afd/ Changeset: 192873018afd User: ggael Date: 2014-10-20 13:55:32+00:00 Summary: Bug 701: workaround (min) and (max) blocking ADL by introducing numext::mini and numext::maxi internal functions and a EIGEN_NOT_A_MACRO macro.
I encountered this issue last night. Glad to see that it is already resolved. Doesn't look like it's made it into a release as of 3.2.5. Have you determined in which release you will include it? Thanks.
We never back-ported this to the 3.2 branch as it seems (and doing so will likely require some manual work). If you are willing to do this, I'd accept a patch/pull-request.
-- 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/701.