Unfortunately windows.h decided to #define min and max. This can lead to numerous difficult to diagnose compiler errors on windows.

In most cases however simply wrapping functions named min or max with parenthesis prevents macro expansion and allows to code to function as intended.

I haven't done a full fix/test but below is the minimal patch required for my project to compile.

Code: Select all

Index: Core/Cwise.h
===================================================================
--- Core/Cwise.h   (revision 72)
+++ Core/Cwise.h   (working copy)
@@ -95,11 +95,11 @@
 
     template
     const EIGEN_CWISE_BINOP_RETURN_TYPE(ei_scalar_min_op)
-    min(const MatrixBase &other) const;
+    (min)(const MatrixBase &other) const;
 
     template
     const EIGEN_CWISE_BINOP_RETURN_TYPE(ei_scalar_max_op)
-    max(const MatrixBase &other) const;
+    (max)(const MatrixBase &other) const;
 
     const EIGEN_CWISE_UNOP_RETURN_TYPE(ei_scalar_abs_op)      abs() const;
     const EIGEN_CWISE_UNOP_RETURN_TYPE(ei_scalar_abs2_op)     abs2() const;
Index: Core/CwiseBinaryOp.h
===================================================================
--- Core/CwiseBinaryOp.h   (revision 72)
+++ Core/CwiseBinaryOp.h   (working copy)
@@ -234,7 +234,7 @@
 template
 template
 EIGEN_STRONG_INLINE const EIGEN_CWISE_BINOP_RETURN_TYPE(ei_scalar_min_op)
-Cwise::min(const MatrixBase &other) const
+(Cwise::min)(const MatrixBase &other) const
 {
   return EIGEN_CWISE_BINOP_RETURN_TYPE(ei_scalar_min_op)(_expression(), other.derived());
 }
@@ -249,7 +249,7 @@
 template
 template
 EIGEN_STRONG_INLINE const EIGEN_CWISE_BINOP_RETURN_TYPE(ei_scalar_max_op)
-Cwise::max(const MatrixBase &other) const
+(Cwise::max)(const MatrixBase &other) const
 {
   return EIGEN_CWISE_BINOP_RETURN_TYPE(ei_scalar_max_op)(_expression(), other.derived());
 }
Index: Core/MathFunctions.h
===================================================================
--- Core/MathFunctions.h   (revision 72)
+++ Core/MathFunctions.h   (working copy)
@@ -126,7 +126,7 @@
 }
 inline bool ei_isApprox(float a, float b, float prec = precision())
 {
-  return ei_abs(a - b) ())
 {
@@ -171,7 +171,7 @@
 }
 inline bool ei_isApprox(double a, double b, double prec = precision())
 {
-  return ei_abs(a - b) ())
 {
@@ -276,7 +276,7 @@
 }
 inline bool ei_isApprox(long double a, long double b, long double prec = precision())
 {
-  return ei_abs(a - b) ())
 {
Index: Core/util/Memory.h
===================================================================
--- Core/util/Memory.h   (revision 72)
+++ Core/util/Memory.h   (working copy)
@@ -332,7 +332,7 @@
 
     size_type max_size() const throw()
     {
-        return std::numeric_limits::max();
+        return (std::numeric_limits::max)();
     }
 
     pointer allocate( size_type num, const_pointer* hint = 0 )
Index: Geometry/AlignedBox.h
===================================================================
--- Geometry/AlignedBox.h   (revision 72)
+++ Geometry/AlignedBox.h   (working copy)
@@ -77,13 +77,13 @@
   }
 
   /** returns the minimal corner */
-  inline const VectorType& min() const { return m_min; }
+  inline const VectorType& (min)() const { return m_min; }
   /** returns a non const reference to the minimal corner */
-  inline VectorType& min() { return m_min; }
+  inline VectorType& (min)() { return m_min; }
   /** returns the maximal corner */
-  inline const VectorType& max() const { return m_max; }
+  inline const VectorType& (max)() const { return m_max; }
   /** returns a non const reference to the maximal corner */
-  inline VectorType& max() { return m_max; }
+  inline VectorType& (max)() { return m_max; }
 
   /** returns true if the point a p is inside the box c *this. */
   inline bool contains(const VectorType& p) const



You can use the preprocessor define NOMINMAX to get rid of min/max defined in #windows.h. I had this problem in another project, NOMINMAX worked just fine.

Do you really need these min/max function macros ? I'd rather recommend to define NOMINMAX before including windows.h.

EDIT: stefanTUD: you're too fast

Maybe we should allow ourselves to #undef min and max in Eigen, like we already #undef a few polluting symbols?

For my project we can get away with NOMINMAX but some projects can't so I thought a solution that is applicable to even projects that rely on windows.h's use of min and max macros would be preferable.

There are only two solutions I'm comfortable with, in order of preference: First, wrapping the min and max usages in eigen with parenthesis to prevent macro expansion. Second, documenting the issue and instructing the user to define NOMINMAX since this really isn't eigen's bug.

I'm not really comfortable with eigen doing "#undef min" and "#undef max" or "#define NOMINMAX" since eigen is a middleware product and this solution assumes specifics about what other libraries are being used alongside it.

Is it really a problem if we #undef min and max?
First, we have to #undef a few stupid symbols anyway, e.g. "vector" that is defined in altivec.h.
Second, a source file #including Eigen is C++, not C, so if they want min they should use std::min. It is really brain dead that windows.h defines min even with __cplusplus. If we force people to use std::min instead of a macro, we're doing them a service, no?[hr]
More arguments:
C and C++ are two distinct language and windows' way of defining min and max is in plain conflict with C++ standard lib, so I don't think we should honor that.

the solution with (min) obfuscates our own source code and there'll always be a place where we forgot the () since ggael and me are on GNU so if we take this way it's going to become another annoying recurrent issue...

I agree that std::min/max should be used where appropriate however you may not be doing someone a service if they have a legacy code base they cannot refactor.

I understand not wanting to obfuscate your source code to work around a shortsighted feature of an OS header. In that case I would prefer a note in the documentation stating that eigen only supports compiling in translation units where windows.h is included if NOMINMAX was defined prior to the inclusion of windows.h. This solution is incompatible with legacy code bases without refactoring but so is the #undef solution. Using #undef leaks to user code and should be avoided in libraries if at all possible.

FWIW Other projects have had this issue too. Boost chose to disable macro expansion[1]. To alleviate the maintenance issue you brought up they have created an automated tool[2] which detects unprotected calls to min and max.

[1]
http://www.boost.org/development/requir ... rogramming
[2]
http://www.boost.org/doc/libs/1_37_0/to ... index.html
https://svn.boost.org/trac/boost/browse ... s/inspect/

There is a simple trick that we can do. We already have header files DisableMSVCWarnings.h included at the beginning of Eigen public headers, and EnableMSVCWarnings included at the end.

We could (rename them and) let them do something like that:

at the beginning,

#ifdef min
#define min_was_defined
#endif
#ifdef max
#define max_was_defined
#endif

and then at the end,

#ifdef min_was_defined
#define min(x,y) blabla....... // copy the junk from windows.h
#endif
....

if different OS headers have the same problem we can still handle that with more #ifdef...

What's your opinion?

Another possibility, which has the advantage of locality: it only needs to be done once at the beginnig of Eigen/Core :

// to put in root namespace, after #including a standard header that defines std::min
#ifdef min
#undef min
using std::min;
#endif

This however relies on the assumption that std::min is actually able to handle all situations that windows's min handles: to be checked.

Finally we just #error if min or max are defined with a hopefully useful message. Tell me if this can be improved.

bjacob wrote:Finally we just #error if min or max are defined with a hopefully useful message. Tell me if this can be improved.

That seems reasonable.