Fixes for compiling in Windows #7
Conversation
|
tested, works fine on ubuntu trusty. +1 |
|
@traversaro can you resolve the conflicts? I think my changes in #14 interfere with some of your changes here. |
| //For using the M_PI macro in visual studio it | ||
| //is necessary to define _USE_MATH_DEFINES | ||
| #ifdef _MSC_VER | ||
| #ifndef _USE_MATH_DEFINE |
There was a problem hiding this comment.
Should this be #ifndef _USE_MATH_DEFINES? It's missing an S
There was a problem hiding this comment.
Definitly. Clearly it was also working with a wrong #ifndef clause, and this is the reason why I did not notice it.
|
I would like to vote on this PR, and also on other fix for |
| @@ -49,6 +49,15 @@ | |||
|
|
|||
| namespace urdf{ | |||
|
|
|||
| //round is not defined in C++98 | |||
| //So in Visual Studio is necessary to define it | |||
| #ifdef _MSC_VER | |||
There was a problem hiding this comment.
I think that we should add a check of version on this. I guess that round was eventually implemented in some recent versions of Visual Studio for C++11 compatibility ( http://www.cplusplus.com/reference/cmath/round/ ).
There was a problem hiding this comment.
It seems std::round was implemented since VS2013. I found some posts that says it's not implemented until VS2012: 1, 2.
There was a problem hiding this comment.
I remember that I was developing this PR to compile a project depending on urdfdom in VS2012, so I think this is make sense.
Conflicts: CMakeLists.txt urdf_model/include/urdf_model/pose.h
|
I solved the conflicts (hope I did not messed up things, it is a bit late here), and I guess it is ok once we add a version check in the At the moment I am bit in an hurry and I cannot solve the conflicts in the other PR, but I have added both of you @scpeters and @jslee02 as collaborators to both the forks repo, so feel free to solve the conflicts if you need this merged soon. |
In Windows find_package(package) finds the PackageConfig.cmake following a set of path patterns [1]: * <prefix>/ * <prefix>/(cmake|CMake)/ * <prefix>/<name>*/ * <prefix>/<name>*/(cmake|CMake)/ This commit modifies the CMake config installation to install them in CMAKE_INSTALL_PREFIX/CMake on Windows (see for reference [2]). [1] : https://cmake.org/cmake/help/v3.0/command/find_package.html [2] : https://github.com/robotology/ycm/blob/master/modules/InstallBasicPackageFiles.cmake#L175
|
I also did a small modification to the directory of installation of CMake config file to make them easier to find for a default installation, see traversaro@bb40954 . |
| #include <string> | ||
| #include <sstream> | ||
| #include <vector> | ||
|
|
||
| #include <cmath> |
There was a problem hiding this comment.
<cmath> is already included on line 53
|
+1 |
| #if (_MSC_VER <= 1700) | ||
| double round(double value) | ||
| { | ||
| return (value >= 0.0f)?(floor(value + 0.5f)):(ceil(value - 0.5f)); |
There was a problem hiding this comment.
instead of defining this, why not just add 0.5 to the arg of round and call floor() instead?
There was a problem hiding this comment.
they give different results:
round(0.5) = 1.0
round(-0.5) = -1.0
floor(0.5 + 0.5) = 1.0
floor(-0.5 + 0.5) = 0.0
There was a problem hiding this comment.
good point, thanks! i guess we care about negative values :)
In this PR there are three fixes for using the library in Windows (tested in Visual Studio 12/Windows 7):
M_PIconstant, not defined by the C++98 standard.roundfunction, not defined by the C++98 standard.