New user self-registration is disabled due to spam. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Before reporting a bug, please make sure that your Eigen version is up-to-date!
Bug 701 - Prohibit macro expansion of min/max without losing Argument Dependent Lookup
Summary: Prohibit macro expansion of min/max without losing Argument Dependent Lookup
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: 3.2
Hardware: All All
: High major
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 3.3
  Show dependency treegraph
 
Reported: 2013-11-06 11:38 UTC by Alexander Werner
Modified: 2015-08-19 19:28 UTC (History)
4 users (show)



Attachments
Patch to resolve the problem (34.21 KB, patch)
2013-11-06 11:38 UTC, Alexander Werner
no flags Details | Diff

Description Alexander Werner 2013-11-06 11:38:52 UTC
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.
Comment 1 Christoph Hertzberg 2013-11-06 13:02:16 UTC
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.
Comment 2 Alexander Werner 2013-11-06 13:23:20 UTC
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.
Comment 3 Christoph Hertzberg 2013-11-06 13:45:52 UTC
(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.
Comment 4 Christoph Hertzberg 2013-11-08 14:06:41 UTC
(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.
Comment 5 Gael Guennebaud 2013-11-09 10:45:23 UTC
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.
Comment 6 Christoph Hertzberg 2013-11-09 12:51:26 UTC
(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.
Comment 7 Gael Guennebaud 2013-11-09 13:00:58 UTC
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".
Comment 8 Christoph Hertzberg 2013-11-09 14:01:32 UTC
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 ...
Comment 9 Gael Guennebaud 2014-10-20 15:56:38 UTC
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.
Comment 10 Curtis Gehman 2015-08-19 17:35:18 UTC
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.
Comment 11 Christoph Hertzberg 2015-08-19 19:28:53 UTC
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.

Note You need to log in before you can comment on or make changes to this bug.