New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error Compiling Triangular_kernel.hpp on Windows #422

Closed
theSundayProgrammer opened this Issue Mar 20, 2015 · 12 comments

Comments

Projects
None yet
2 participants
@theSundayProgrammer
Copy link
Contributor

theSundayProgrammer commented Mar 20, 2015

In triangular_kernel.hpp (line 657) the following function fails to compile as it is using the max macro from a windows header.
double Evaluate(const double distance) const
{
return std::max(0.0, (1 - distance) / bandwidth);
}

The workaround is to add

ifdef max

undef max

endif

close to the top of the file

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Mar 20, 2015

This is already done in src/mlpack/core.hpp; see the end of the file:

https://github.com/mlpack/mlpack/blob/master/src/mlpack/core.hpp

If mlpack is actually failing to compile, please attach a build log so I can dig deeper, but this is a problem that should not be happening.

@rcurtin rcurtin closed this Mar 20, 2015

@rcurtin rcurtin added the R: fixed label Mar 20, 2015

@theSundayProgrammer

This comment has been minimized.

Copy link
Contributor

theSundayProgrammer commented Mar 20, 2015

#include <mlpack/core/kernels/triangular_kernel.hpp>

// Use Armadillo's C++ version detection.
#ifdef ARMA_USE_CXX11
#define MLPACK_USE_CX11
#endif

#endif

// Clean up unfortunate Windows preprocessor definitions, even if this file was
// already included. Use std::min and std::max!
#ifdef _WIN32
#ifdef min
#undef min
#endif

#ifdef max
#undef max
#endif


this must be done before including triangular_kernel.hpp not after as is done in core.hpp

@theSundayProgrammer

This comment has been minimized.

Copy link
Contributor

theSundayProgrammer commented Mar 20, 2015

error compiling lars.cpp

Error 7 error C2059: syntax error : '::' mlpack\mlpack-1.0.12\src\mlpack\core\kernels\triangular_kernel.hpp 64 1 mlpack
Error 6 error C2589: '(' : illegal token on right side of '::' mlpack\mlpack-1.0.12\src\mlpack\core\kernels\triangular_kernel.hpp 64 1 mlpack

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Mar 20, 2015

Ah, I see now. I don't have a working Visual Studio environment so I have a difficult time testing.

@rcurtin rcurtin reopened this Mar 20, 2015

@rcurtin rcurtin added T: defect and removed R: fixed labels Mar 20, 2015

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Mar 20, 2015

Can you update master to a120654 and test the fix I've committed? Thanks!

@theSundayProgrammer

This comment has been minimized.

Copy link
Contributor

theSundayProgrammer commented Mar 20, 2015

That particular fix does not work. But I can see what you are trying. You need to include those #undef's after including windows headers. I'll try to locate that and let you know in a day or two.

@theSundayProgrammer

This comment has been minimized.

Copy link
Contributor

theSundayProgrammer commented Mar 20, 2015

I tried this in timers.hpp and it worked:
#elif defined(_WIN32)
#define NOMINMAX ///////The Sunday Programmer added this
#include <windows.h> //GetSystemTimeAsFileTime(),

@theSundayProgrammer

This comment has been minimized.

Copy link
Contributor

theSundayProgrammer commented Mar 20, 2015

On further investigation I see no need to include platform dependent headers in timers.hpp. I would recommend moving the entire
#if defined(unix) || defined(__unix)
...
#else
#error "unknown OS"
#endif

to timers.cpp from timers.hpp.

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Mar 20, 2015

Ah, okay, now I understand better. The first patch was written on a plane waiting to take off so I'm not too surprised it didn't work perfectly. :)

I've taken a closer look at the situation and committed 8bd3f96. This removes the undef'ing of min/max entirely and sets NOMINMAX safely before including windows.h, like you suggested. Can you try it and see if that fixes the issue? Thanks for testing the previous patch, also.

I don't see a quick way to move the inclusion of the system-dependent headers into timers.cpp, since the Timers class exposes some methods and has some members that require a definition of the timeval type. If you'd like to refactor and submit a pull request, I'm happy to go that direction, but I'm also happy enough to leave it as-is.

@theSundayProgrammer

This comment has been minimized.

Copy link
Contributor

theSundayProgrammer commented Mar 21, 2015

Thanks. That works on Visual Studio 2013 release x64.

Off topic: just to define 'timeval' we have to include <windows.h> and <winsock.h>. If we can avoid that we could save a lot of compilation time.

@theSundayProgrammer

This comment has been minimized.

Copy link
Contributor

theSundayProgrammer commented Mar 21, 2015

Off topic: just to define 'timeval' we have to include windows.h and winsock.h. If we can avoid that we could save a lot of compilation time.

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Mar 21, 2015

Cool; thanks for testing that. I'm going to close this ticket then, and open another one for the off-topic issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment