COIN-OR::LEMON - Graph Library

Opened 10 years ago

Closed 10 years ago

#199 closed defect (fixed)

std::isnan() is not available with all compiler

Reported by: Alpar Juttner Owned by: Alpar Juttner
Priority: major Milestone: LEMON 1.1 release
Component: core Version: hg main
Keywords: Cc:
Revision id: 04c0631fd332

Description

std::isnan() is not provided by some compiler, such as VS2005 and icc-10.1.

We should provide an own implementation like this:

    inline bool isnan(double v)
    {
      return v!=v;
    }

Of course, this function can optionally use std::isnan() if it is available.

Attachments (2)

03ce6078f568.patch (3.1 KB) - added by Alpar Juttner 10 years ago.
81627fa1b007.patch (2.5 KB) - added by Alpar Juttner 10 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 10 years ago by Alpar Juttner

Update on Intel C++:

My previous failure with icc turned out to be a linking problem due to a corrupt installation of icc. At least I could successfully run make check on [04c0631fd332] with the 64 bit version of icc, both with version 10.1-021 and 11.0-074. (Although the compilation is extremely slow.)

comment:2 in reply to:  1 ; Changed 10 years ago by Alpar Juttner

Replying to alpar:

Update on Intel C++:

My previous failure with icc turned out to be a linking problem due to a corrupt installation of icc. At least I could successfully run make check on [04c0631fd332] with the 64 bit version of icc, both with version 10.1-021 and 11.0-074. (Although the compilation is extremely slow.)

I find now that it may depend on the gcc version installed (icc uses the gcc header files).

  • At the successful compilation above the default gcc version is 4.1.0, so icc probably uses the headers of this version.
  • On the other hand on my laptop icc uses the headers of gcc 4.3.2 and emits the following error.
    /usr/include/c++/4.3/cmath(540): error: identifier "__builtin_isnan" is undefined
            return __builtin_isnan(__type(__f));
                   ^
              detected during instantiation of "__gnu_cxx::__enable_if<std::__is_arithmetic<_Tp>::__value, int>::__type std::isnan(_Tp) [with _Tp=double]" at line 600 of "../lemon/lp_base.h"
    
    compilation aborted for ../lemon/lp_skeleton.cc (code 2)
    

Anyway, the attached patch ([03ce6078f568]) resolves this problem both on icc and VS2005. There are some question coming with this patch.

  • Applying this patch, lemon will use the custom implementation even if std::isnan() is available. Shouldn't we use that one if available? Does is that have any practical advantage of doing that?
  • Will this own isnan() implementation work correctly on all platforms?
  • Currently, I put this stuff into a separate file (lemon/bits/isnan.h). Shouldn't it go into say lemon/core.h instead?

Changed 10 years ago by Alpar Juttner

Attachment: 03ce6078f568.patch added

comment:3 in reply to:  2 ; Changed 10 years ago by Balazs Dezso

Replying to alpar:

Replying to alpar:

Update on Intel C++:

My previous failure with icc turned out to be a linking problem due to a corrupt installation of icc. At least I could successfully run make check on [04c0631fd332] with the 64 bit version of icc, both with version 10.1-021 and 11.0-074. (Although the compilation is extremely slow.)

I find now that it may depend on the gcc version installed (icc uses the gcc header files).

  • At the successful compilation above the default gcc version is 4.1.0, so icc probably uses the headers of this version.
  • On the other hand on my laptop icc uses the headers of gcc 4.3.2 and emits the following error.
    /usr/include/c++/4.3/cmath(540): error: identifier "__builtin_isnan" is undefined
            return __builtin_isnan(__type(__f));
                   ^
              detected during instantiation of "__gnu_cxx::__enable_if<std::__is_arithmetic<_Tp>::__value, int>::__type std::isnan(_Tp) [with _Tp=double]" at line 600 of "../lemon/lp_base.h"
    
    compilation aborted for ../lemon/lp_skeleton.cc (code 2)
    

Anyway, the attached patch ([03ce6078f568]) resolves this problem both on icc and VS2005. There are some question coming with this patch.

  • Applying this patch, lemon will use the custom implementation even if std::isnan() is available. Shouldn't we use that one if available? Does is that have any practical advantage of doing that?

In the gcc there is a _GLIBCXX_HAVE_ISNAN macro which can indicate whether the isnan function is exists. If the macro is set we can use the gcc version. However, the performance of the two variants with gcc 4.3.3 are quite same.

  • Will this own isnan() implementation work correctly on all platforms?

I think yes, the only number which differs from itself is the nan.

  • Currently, I put this stuff into a separate file (lemon/bits/isnan.h). Shouldn't it go into say lemon/core.h instead?

Maybe into the lemon/math.h.

Changed 10 years ago by Alpar Juttner

Attachment: 81627fa1b007.patch added

comment:4 in reply to:  3 ; Changed 10 years ago by Alpar Juttner

Replying to deba:

  • Currently, I put this stuff into a separate file (lemon/bits/isnan.h). Shouldn't it go into say lemon/core.h instead?

Maybe into the lemon/math.h.

So I moved it to lemon/math.h81627fa1b007], see [81627fa1b007]. I suggest putting this changeset into the main. What do you think?

Note that LEMON will then use its own implementation even if std::isnan() is available. We may change it later if we see any good reason for doing so.

comment:5 in reply to:  4 ; Changed 10 years ago by Balazs Dezso

Replying to alpar:

Replying to deba:

  • Currently, I put this stuff into a separate file (lemon/bits/isnan.h). Shouldn't it go into say lemon/core.h instead?

Maybe into the lemon/math.h.

So I moved it to lemon/math.h81627fa1b007], see [81627fa1b007]. I suggest putting this changeset into the main. What do you think?

Note that LEMON will then use its own implementation even if std::isnan() is available. We may change it later if we see any good reason for doing so.

In my opinion, it can go to the main.

comment:6 in reply to:  5 Changed 10 years ago by Alpar Juttner

Resolution: fixed
Status: newclosed

Replying to deba:

In my opinion, it can go to the main.

Done.

Note: See TracTickets for help on using tickets.