isfinite compilation error

[connormanning]

Building CXX object myProject/third/CMakeFiles/third.dir/jsoncpp.cpp.o
/home/vagrant/myProject/myProject/third/jsoncpp.cpp: In function ‘std::string Json::valueToString(double)’:
/home/vagrant/myProject/myProject/third/jsoncpp.cpp:4049:21: error: ‘isfinite’ was not declared in this scope
   if (isfinite(value)) {
                     ^
/home/vagrant/myProject/myProject/third/jsoncpp.cpp:4049:21: note: suggested alternative:
In file included from /usr/include/c++/4.8/random:38:0,
                 from /usr/include/c++/4.8/bits/stl_algo.h:65,
                 from /usr/include/c++/4.8/algorithm:62,
                 from /home/vagrant/myProject/myProject/third/jsoncpp.cpp:2398:
/usr/include/c++/4.8/cmath:596:5: note:   ‘std::isfinite’
     isfinite(_Tp __x)
     ^
make[2]: *** [myProject/third/CMakeFiles/third.dir/jsoncpp.cpp.o] Error 1
make[1]: *** [myProject/third/CMakeFiles/third.dir/all] Error 2
make: *** [all] Error 2

This is on Ubuntu Trusty, failing here (I am using an amalgamated version of SHA ed495ed). For me, replacing isfinite() with std::isfinite() at that line let me compile. However due to the cross-platform support for isfinite here this isn't the real solution.

[Dani-Hub]

What is the gcc version you are using? Is it possible that you are a victim of this error:

https://bugs.launchpad.net/ubuntu/+source/gcc-4.3/+bug/512741

[connormanning]

Interesting find, wasn't aware of that. My GCC version is 4.8.2.

$ gcc -v
Using built-in specs.
COLLECT_GCC=g++
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.8/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 4.8.2-19ubuntu1' --with-bugurl=file:///usr/share/doc/gcc-4.8/README.Bugs --enable-languages=c,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.8 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.8 --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --enable-gnu-unique-object --disable-libmudflap --enable-plugin --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-4.8-amd64/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-4.8-amd64 --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-4.8-amd64 --with-arch-directory=amd64 --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --enable-objc-gc --enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 4.8.2 (Ubuntu 4.8.2-19ubuntu1)

Also, running the test program mentioned in the bug report does not give the failed result for me so I believe I am safe from that GCC bug:

$ gcc test.c // All compiler flag combinations gave the identical results below:
test.c:4:2: error: #error isfinite defined
 #error isfinite defined

I always got the defined result for all combinations of g++/gcc and -std=c++11/-std=c99/(no std flags).

[Dani-Hub]

I partially can understand the problem now and there is a simple reproducer for this:

#include <math.h>
#include <cmath>
#include <math.h>

int main() {
  double value = 12.3;
  if (isfinite(value)) {}
}

This leads to the same kind of error that you observed and this looks like a compiler defect to me. But there is a simple workaround for this. Instead of using std::isfinite (which will not work for other compilers such as Visual Studio), just add a function-local using-directive as follows:

#include <math.h>
#include <cmath>
#include <math.h>

int main() {
  double value = 12.3;
  using namespace std; // Insert this before attempting to invoke isfinite
  if (isfinite(value)) {}
}

To make this idiom more manageable, I suggest to add the following inline function to the jsoncpp code:

inline bool isfinite_impl(double x)
{
  using namespace std;
  return isfinite(x) != 0;
}

and to refer to that isfinite_impl function instead in the code.

[Dani-Hub]

My recent commit (#217) should hopefully resolve the problem. You might want to test it as well.

[connormanning]

Yes I believe that would resolve it. I also have an open pull request for the issue at #215.

I'm hoping for some input from @cdunn2001 about the usage of things like math.h instead of cmath in this file. From what I can tell from a quick browse around the jsoncpp source, most of the other files prefer using the C++ std header (e.g. here).

What I'd like to see is using the C++ headers everywhere - I'm wondering if there's a reason that this file uses the C style of include or if it just hasn't been switched over yet. I could make those changes if wanted.

In its current state I think your pull request is better - however if the preference is to switch this file over to the std headers, then I think my pull request fits that model nicely.

[Dani-Hub]

I have studied your pull request but I can ensure you that - albeit it works for recent gcc versions - it would not work for Visual Studio (which depends on _finite not being in namespace std) and presumably it would also not work for the special sun case referred to in the same source file:

#if defined(__sun) && defined(__SVR4) //Solaris
#include <ieeefp.h>
#define isfinite finite
#endif

which also relies on remapping isfinite onto a function that is not part of namespace std.

[connormanning]

Can you explain why? I have only added the mapping into namespace std in the case that the compiler is not Visual Studio nor Solaris. For those two cases, I have left the macro for isfinite with the same definition that they had before my pull request.

Only in the case that we are not Visual Studio and are not Solaris, then I map isfinite to std::isfinite.

[Dani-Hub]

Sorry, I was a bit hasty when arguing about your resolution. It fixes the problem for the sun case as well, but it does not activate the isfinite mapping for Visual Studio (That is one relevant side aspect of my own pull request). At the very moment the difference doesn't matter much, but the isfinite test will be needed in the near future during my fix of #209, which needs an accessible isfinite function for all systems.

[connormanning]

There are two separate issues at play. The issue I solved was to make the current code work and to defer to a well-defined isfinite, without changing the existing activation logic at all. You solved that issue, as well as the separate issue of changing the MSVC versions at which the fix is activated, which was not the intent of my pull request.

Personally I still think the file should be pulling in cmath instead of math.h, along with all the other similar standard headers, for consistency with the rest of the JsonCpp codebase. This would avoid the odd using statement and make things more well-defined for non-MSVC/Sun compilers.

Although functionally your method of using namespace std is fine, the fact that your isfinite_impl isn't invoking a well-defined header doesn't sit well with me. Depending on whether the project has previously included cmath or not, your function calls into different headers, and even though nothing will break that just doesn't seem right to me.

[cdunn2001]

I'm hoping for some input from @cdunn2001 about the usage of things like math.h instead of cmath in this file. From what I can tell from a quick browse around the jsoncpp source, most of the other files prefer using the C++ std header.

It's historical. However, I always worry about breaking users with old compilers. Ideally, the older branch will not include these new features. I'm fine with <cmath> for the master branch.

All the #ifdefs are becoming unwieldy. Let's try to combine most of that into one place, and then rebase these inf changes onto that. Does that make sense?

[Dani-Hub]

So is the intention here to break compatibility with Visual Studio including VS 2012? Because that will be the result when following your directions exactly, because even though VS 2012 provides header <cmath>, this does not provide neither std::isfinite nor a isfinite macro or function, so the special conditional code that I was suggesting would still be needed. It seems to me to be a very unfortunate decision to cut off that compiler given its popularity. This would be show stopper for me.

[cdunn2001]

I agree. We want VS2012 to work with the master branch.

[connormanning]

@Dani-Hub, I am confused about your opposition to including <cmath>, since the MSVC preprocessing needs to include <float.h> to get the _finite functionality - so the #includes used for the GCC case don't seem to affect you. Why would using cmath for the GCC case break VS compatibility? The MSVC preprocessing logic branch would maintain the functionality of what you proposed.

As far as I can tell, every option discussed would maintain the entire functionality of your pull request, if not slightly different aesthetically.

[connormanning]

@Dani-Hub please take a look at #218 I just made - does that meet your functionality needs? This is what I've been proposing in my last few posts, and I figured it's easier to discuss actual code than concepts.

[Dani-Hub]

My concerns had always been regarding breaking support for VS 2012, and not specifically against including cmath instead of <math.h>.

I have two concerns with the recently provided patch:

1. The logic to decide for a replacement of snprintf should not be part of the same (now changed) test regarding isfinite, so this one is broken:

#if defined(_MSC_VER) && _MSC_VER >= 1200 // VC++ 6.0 and above
#include <float.h>
#define snprintf _snprintf
#endif

Yesterday I found the finally Visual Studio 2013 will provide support for std::isfinite, so this one should be

#if defined(_MSC_VER) && _MSC_VER >= 1200 && _MSC_VER < 1800 // Between VC++ 6.0 and VC++ 11.0
#include <float.h>
#endif

instead.

2. Furthermore the following additional test

#if defined(_MSC_VER) && _MSC_VER < 1500 // VC++ 8.0 and below
#include <float.h>
#define snprintf _snprintf
#endif

should not include <float.h>, because it only affects _snprintfwhich is not part of that header.

That being said, if I above changes would be applied, the patch looks good to me.

[connormanning]

I've incorporated the suggested changes into #218, and fixed my error from the previous commit.

[Dani-Hub]

I like your changes versus isfinite - thanks!
But I'm a bit concerned regarding the other header changes to the proper <c***> forms: As far as I see the code now includes <cstring> (for example), but still refers to unqualified snprintf: I believe to make this work, we need the same macro trick as you already did for isfinite.

[cdunn2001]

Dani-Hub, if we need another correction, please submit another PR. You're probably right, but I've lost track of the issues.