jrl-umi3218 / jrl-mal

Matrix Abstract Layer to allow multiple libraries on JRL code
GNU Lesser General Public License v3.0
5 stars 5 forks source link

isnan portability #4

Closed aelkhour closed 12 years ago

aelkhour commented 12 years ago

Hi,

I have pushed a new branch called topic/isnan-portability for the following reasons:

Since boost is already a dependancy of jrl-mal, calling boost::math::isnan would make the code portable without any additionnal tests (I think).

I made a test file that calls isnan, but I only installed jrl-mal on ubuntu 10.04. Can anyone who has Windows or MAC OS try to install it and see if it works?

Thanks! Antonio El Khoury

olivier-stasse commented 12 years ago

Dear Antonio, Thank you very much for your proposal. I pushed on topic/python the modification you proposed plus some adjustement to keep something specific to boost (an eigen version will be the next milestone). And cherry-picked the fixes from master to topic/python. Could you verify it is fine for you ? If it is I will make topic/python the next master. Best Regards.

francois-keith commented 12 years ago

Hello

This patch crashes the compilation with windows of other projects (e.g. GenerateRobotForMaple.cpp (in jrl-dynamics) ), since the symbol isnan is not defined anymore: the following line has been suppressed:

#   define isnan _isnan

In order to apply this patch, the solution, quite simple, is to replace every isnan by boost::math::isnan in every project... or at least those used by windows, since this modification does not prevent the compilation with mac os x. This modification does not seem to long to do, so if your patch is merged with the master, I will handle it.

In any case, it is better to use boost::math::isnan to ensure the compatibility between the platform.

François

PS: thanks for thinking at the portability.

francois-keith commented 12 years ago

Well, after some thorough tests, it seems that the aforementioned file is the only one crashing. So the correction is quite easy.

olivier-stasse commented 12 years ago

Hi Francois and Antonio, Thanks for your efforts in keeping the system portable. So the idea is to make boost mandatory for isnan, which I agree with. However we have to rewrite a bit the code to make the choice between ublas and possible other libraries (such as eigen).

thomas-moulard commented 12 years ago

+1, even after switching to eigen we will still need Boost in about every package so I don't think it is a problem.

olivier-stasse commented 12 years ago

I pushed a modification of jrl-dynamics to fix the matter

aelkhour commented 12 years ago

Hi everyone,

Thank you for your quick answers.

@Olivier: I pulled topic/python in jrl-mal and it seems to work.

@François: Portability was actually a side-effect, but I'm glad it helped! :)

@Thomas: Thanks for the idea (originally his).

Antonio