Fix infinite compilation problems on some gcc variants (Issue #214)

[Dani-Hub]

The provided patch has been tested on gcc mingw 5.0.0 and for Visual Studio 2012.

[cdunn2001]

Where are we on this?

[Dani-Hub]

I was quite busy for a while with other things and will proceed somewhen during or at the end of this week

[cdunn2001]

Don't worry about the appveyor error. Not sure, but it might be simply that you don't have appveyor.yml yet. Ignore it.

You must fix the Travis error:

Testing BuilderTest/settings: FAILED
Testing IteratorTest/distance: OK
Testing IteratorTest/names: OK
Testing IteratorTest/indexes: OK
* Detail of BuilderTest/settings test failure:
/home/travis/build/open-source-parsers/jsoncpp/src/test_lib_json/main.cpp(2317): true == wb.validate(&errs)
  Expected: true
  Actual  : false

Also, you need to rebase on the tip of master. You have merged instead. Don't merge! Rebase!

I would help you with this, but my laptop was stolen this week. I don't yet have a development environment, aside from my company's servers.

[Dani-Hub]

My bad - I corrected the oversight and pushed it as well. Except for the AppVeyor errors it seems to look ok to me now. In regard to your stolen laptop I can feel the pain - ouch! I'm also sorry for the trouble I'm causing with the merge - albeit I'm using git since years this is my first experience with working on a fork, I really have severe problems understanding the concepts behind that approach. As you suggested I tried to rebase, but Tortoise says now "Nothing need rebase - master equal remotes/origin/master". Isn't this state correct?

[cdunn2001]

#247 is your code, sans the snprintf stuff (which is a separate issue), rebased onto my appveyor branch (to get appveyor.yml) excluding dups which were already incorporated into master. I did this:

$ git checkout -b nan Dani-Hub/master
$ git rebase --onto cdunn2001/appveyor 7cea2d3

That means "rebase the current branch onto the appveyor branch of the cdunn2001 remote, but only include commits down-to (and not including) 7cea2d3", which happens to be your most recent merge commit from origin/master. appveyor passed most tests, but failed this:

  * Detail of ValueTest/specialFloats test failure:
  C:\projects\jsoncpp\src\test_lib_json\main.cpp(1648): expected == result
    Expected: 'NaN'
    Actual  : '1.#QNAN'
  C:\projects\jsoncpp\src\test_lib_json\main.cpp(1653): expected == result
    Expected: 'Infinity'
    Actual  : '1.#INF'
  C:\projects\jsoncpp\src\test_lib_json\main.cpp(1658): expected == result
    Expected: '-Infinity'
    Actual  : '-1.#INF'
  52/53 tests passed (1 failure(s))
C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V120\Microsoft.CppCommon.targets(132,5): error MSB3073: The command "setlocal [C:\projects\jsoncpp\src\test_lib_json\jsoncpp_test.vcxproj]
C:\Program Files (x86)\MSBuild\Micro

(#248 is a PR for your request without appveyor. I rebased the same code to origin/master (which currently lacks the appveyor commit). I just want to repro your appveyor problem before pulling apveyor.yml.)

You will have much more fun with git if you learn how to rebase. I don't know how to do that with Tortoise. I use shell commands. If you don't know how to rebase, then you should throw away your current branch and pull from the nan branch in my own repo, cdunn2001/jsoncpp.

At any rate, you need to fix the MS failures.

[Dani-Hub]

None of the test cases fails on my side and I'm working with Visual Studio 2012. Why do you think that my additional changes are not relevant - I'm pretty sure they are!

[cdunn2001]

Pull my branch of your code and take a look. (Or just peruse it in the GitHub browser.) Maybe it's missing something. Maybe I did not resolve the conflicts correctly.

[Dani-Hub]

The branch you pointed me too clearly is not the code that I committed in my fork. As I said, the other changes are relevant, too. Sigh - working via forks is not much of a fun to me. Should I maybe try to create a diff file and upload it? I have no good ideas anymore.

[cdunn2001]

You need to learn how to rebase. You can rebase yourself and get whatever you need.

The code here is merged from master several times. That causes confusion. Think of the git history as a series of features. You have "features" which are already incorporated into master. I'd like a series of commit which does not have such code duplication. Maybe I can show you visually...

* 2215e82 (HEAD, dani/master) Added missing "useSpecialFloats" to valid key set of writer
* d8e2e07 Add unit tests for reading and writing with special-float flags activated
* a6e58f9 Extend CharReaderBuilder to support the new setting allowSpecialFloats while reading,
* dc59cac Add new valueToString overloads for doubles with additional parameter denoting output
* 1dddfd4 Add documentation of new internal header
* 578d199 Minor doc improvements (formatting)
* f7f3bdf Add new reader setting allowSpecialFloats to CharReaderBuilder
* 308cd0b Improve comment formatting
* 60c6b71 Merge with master
*   7cea2d3 Merge branch 'master' of github.com:open-source-parsers/jsoncpp
|\
| * 50069d7 (os/master, origin/master, origin/HEAD, master) prefer std::string for setComment()
| * 24682e3 (tag: 1.6.2) 1.6.2 <- 1.6.1
| *   c2b988e Merge pull request #241 from cdunn2001/fix-more-utf8
| |\
| | * e255ce3 (origin/fix-more-utf8) support UTF-8 in old Writers
| |/
| *   779b5bc Merge pull request #239 from sbc100/copyright
| |\
| | * 6386061 Add copyright information to .py files
| |/
| * 9cb88d2 (tag: 1.6.1) 1.6.1 <- 1.6.0
| *   363e51c Merge pull request #232 from cdunn2001/fix-snprintf
| |\
| | * 240ddb6 use std::snprintf for C++11
| | * 9dd77dc Revert "Use std namespace for snprintf."
| |/
| *   244b149 Merge pull request #225 from selaselah/master
| |\
| | * c083835 fix find_program() bug: no result in not-win sys
| |/
| *   cbe7e7c (tag: 1.6.0) Merge pull request #221 from btolfa/forgotten-virtual-dtor
| |\
| | * be183de Update reader.h
| |/
* | 2407e84 Introduce pure macro header that must be used as very last header
* | f2b2105 Introduce single point of definition of snprintf macro and fix current Visual Studi
* | dc2acf1 Sync with master
* |   7ad02f3 Remove isfiniteImpl function again
|\ \
| |/
| *   951bd3d Merge pull request #219 from cdunn2001/c-std-headers
| |\
| | * 1c58876 Use std namespace for snprintf.
| | * 2f20346 Constrain MSVC _isfinite to before 2013, remove duplicate includes.
| | * 7020451 Fix isfinite for MSVC.
| | * 80497f1 Use C++ standard headers.
| |/
* |   bf32034 Merge remote-tracking branch 'upstream/master'
|\ \
| |/
| * f9feb66 Change exception data member
* | f3b8b44 Change exception data member from "reference to string" to "string" (Resolves the m
* | cb546c9 Fix isfinite compilation error for gcc (#214) and provide isfinite functionality fo
* |   9554e01 Merge remote-tracking branch 'upstream/master'
|\ \
| |/
| * ed495ed prefer ValueIterator::name() to ::memberName()
| *   3c0a383 Merge pull request #212 from cdunn2001/macro-deprec
| |\
| | * 5003983 Make preprocessor query robust against older gcc versions
| | * 871b311 Provide JSONCPP_DEPRECATED definitions for clang and gcc
| |/
| * cdbc35f 1.6.0
| * 4e30c4f comments
| *   0d33cb3 Merge pull request #211 from cdunn2001/except
| |\
| | * 2250b3c use Json::RuntimeError
| | * 9376368 use Json::LogicError in macros
| | * 5383794 Runtime/LogicError and throwers
| | * 75279cc base Json::Exception
| | * 717b086 clarify errors
| |/
* | 163f2bf Make preprocessor query robust against older gcc versions
* | f399c40 Provide JSONCPP_DEPRECATED definitions for clang and gcc
|/
* ee4ea0e (tag: 1.5.4) delete debug code from test

See? Your code is based off 1.5.4. You have several merges from master, and now I cannot tell which of the released features are properly integrated into your code.

You could also diff between yours and mine. Here:

• Dani-Hub/jsoncpp@master...cdunn2001:nan-sans-appveyor

Here, this PR will give us a place to discuss the actual diffs. Don't actually pull it. Just comment on the diffs:

• Nan sans appveyor Dani-Hub/jsoncpp#1

[cdunn2001]

Hmmm. I don't understand what happened in that rebase. You're right: A lot of your changes are gone. I'll try it again... Even for the comparison, GitHub does not show the diffs properly. I don't know why yet. Anyway, here are the diffs:

$ git diff -w nan-sans-appveyor  dani/master
diff --git include/json/writer.h include/json/writer.h
index 78e114d..a6e5bb8 100644
--- include/json/writer.h
+++ include/json/writer.h
@@ -99,6 +99,10 @@ public:
         Strictly speaking, this is not valid JSON. But when the output is being
         fed to a browser's Javascript, it makes for smaller output and the
         browser can handle the output just fine.
+    - "useSpecialFloats": false or true
+         - If true, outputs non-finite floating point values in the following way:
+           NaN values as "NaN", positive infinity as "Infinity", and negative infinity
+               as "-Infinity".

diff --git src/jsontestrunner/main.cpp src/jsontestrunner/main.cpp
index 1ec1fb6..30d6f43 100644
--- src/jsontestrunner/main.cpp
+++ src/jsontestrunner/main.cpp
@@ -26,7 +26,7 @@ struct Options

 static std::string normalizeFloatingPointStr(double value) {
   char buffer[32];
-#if defined(_MSC_VER) && defined(__STDC_SECURE_LIB__)
+#if defined(_MSC_VER)
   sprintf_s(buffer, sizeof(buffer), "%.16g", value);
 #else
   snprintf(buffer, sizeof(buffer), "%.16g", value);
diff --git src/lib_json/json_macros.h src/lib_json/json_macros.h
new file mode 100644
index 0000000..1d87532
--- /dev/null
+++ src/lib_json/json_macros.h
@@ -0,0 +1,38 @@
+// Copyright 2007-2010 Baptiste Lepilleur
+// Distributed under MIT license, or public domain if desired and
+// recognized in your jurisdiction.
+// See file LICENSE for detail or copy at http://jsoncpp.sourceforge.net/LICENSE
+
+#ifndef LIB_JSONCPP_JSON_MACROS_H_INCLUDED
+#define LIB_JSONCPP_JSON_MACROS_H_INCLUDED
...
+#endif // LIB_JSONCPP_JSON_MACROS_H_INCLUDED
diff --git src/lib_json/json_reader.cpp src/lib_json/json_reader.cpp
index 9528c4e..274db3a 100644
--- src/lib_json/json_reader.cpp
+++ src/lib_json/json_reader.cpp
@@ -19,9 +19,9 @@
 #include <set>
 #include <limits>

-#if defined(_MSC_VER) && _MSC_VER < 1500 // VC++ 8.0 and below
-#define snprintf _snprintf
-#endif
+// This must be the last include, because otherwise due to its macro
+// definitions it could change the semantics of other headers:
+#include "json_macros.h"

 #if defined(_MSC_VER) && _MSC_VER >= 1400 // VC++ 8.0
 // Disable warning about strdup being deprecated.
@@ -818,15 +818,7 @@ std::string Reader::getLocationLineAndColumn(Location location) const {
   int line, column;
   getLocationLineAndColumn(location, line, column);
   char buffer[18 + 16 + 16 + 1];
-#if defined(_MSC_VER) && defined(__STDC_SECURE_LIB__)
-#if defined(WINCE)
-  _snprintf(buffer, sizeof(buffer), "Line %d, Column %d", line, column);
-#else
-  sprintf_s(buffer, sizeof(buffer), "Line %d, Column %d", line, column);
-#endif
-#else
   snprintf(buffer, sizeof(buffer), "Line %d, Column %d", line, column);
-#endif
   return buffer;
 }

@@ -1837,15 +1829,7 @@ std::string OurReader::getLocationLineAndColumn(Location location) const
   int line, column;
   getLocationLineAndColumn(location, line, column);
   char buffer[18 + 16 + 16 + 1];
-#if defined(_MSC_VER) && defined(__STDC_SECURE_LIB__)
-#if defined(WINCE)
-  _snprintf(buffer, sizeof(buffer), "Line %d, Column %d", line, column);
-#else
-  sprintf_s(buffer, sizeof(buffer), "Line %d, Column %d", line, column);
-#endif
-#else
   snprintf(buffer, sizeof(buffer), "Line %d, Column %d", line, column);
-#endif
   return buffer;
 }

diff --git src/lib_json/json_value.cpp src/lib_json/json_value.cpp
index 8fbce54..35f0087 100644
--- src/lib_json/json_value.cpp
+++ src/lib_json/json_value.cpp
@@ -19,6 +19,10 @@
 #include <cstddef> // size_t
 #include <algorithm> // min()

+// This must be the last include, because otherwise due to its macro
+// definitions it could change the semantics of other headers:
+#include "json_macros.h"
+
 #define JSON_ASSERT_UNREACHABLE assert(false)

 namespace Json {
diff --git src/lib_json/json_writer.cpp src/lib_json/json_writer.cpp
index c15991b..2aea575 100644
--- src/lib_json/json_writer.cpp
+++ src/lib_json/json_writer.cpp
@@ -16,22 +16,9 @@
 #include <cstring>
 #include <cstdio>

-#if defined(_MSC_VER) && _MSC_VER >= 1200 && _MSC_VER < 1800 // Between VC++ 6.0 and VC++ 11.0
-#include <float.h>
-#define isfinite _finite
-#elif defined(__sun) && defined(__SVR4) //Solaris
-#include <ieeefp.h>
-#define isfinite finite
-#else
-#include <cmath>
-#define isfinite std::isfinite
-#endif
-
-#if defined(_MSC_VER) && _MSC_VER < 1500 // VC++ 8.0 and below
-#define snprintf _snprintf
-#elif __cplusplus >= 201103L
-#define snprintf std::snprintf
-#endif
+// This must be the last include, because otherwise due to its macro
+// definitions it could change the semantics of other headers:
+#include "json_macros.h"

 #if defined(_MSC_VER) && _MSC_VER >= 1400 // VC++ 8.0
 // Disable warning about strdup being deprecated.
@@ -106,15 +93,6 @@ std::string valueToString(double value, bool useSpecialFloats) {
   // Print into the buffer. We need not request the alternative representation
   // that always has a decimal point because JSON doesn't distingish the
   // concepts of reals and integers.
-#if defined(_MSC_VER) && defined(__STDC_SECURE_LIB__) // Use secure version with
-                                                      // visual studio 2005 to
-                                                      // avoid warning.
-#if defined(WINCE)
-  len = _snprintf(buffer, sizeof(buffer), "%.17g", value);
-#else
-  len = sprintf_s(buffer, sizeof(buffer), "%.17g", value);
-#endif
-#else
   if (isfinite(value)) {
     len = snprintf(buffer, sizeof(buffer), "%.17g", value);
   } else {
@@ -128,7 +106,7 @@ std::string valueToString(double value, bool useSpecialFloats) {
     }
     // For those, we do not need to call fixNumLoc, but it is fast.
   }
-#endif
+

It's hard to see without colors, but hopefully you can see what's different. The main thing is the removal of your new file, src/lib_json/json_macros.h. I am trying to separate the features. If you want to change snprintf or move macros, please do that in a separate PR. We need to discuss and test the changes needed for NaN/Infinity in a PR with minimal changes.

Also, learn git rebase. It's worth the effort. A linear history helps tremendously.

[Dani-Hub]

What is the advantage of commenting on Dani-Hub#1 ? I do know that it's broken, because my implementation was exactly written to fix the previous limitations of the HEAD code, since neither it nor Dani-Hub#1 does respect the isfinite evaluation for Visual Studio. I don't understand what kind of proposal you would accept and especially how I can make it available to you.
In regard to rebasing I have described that my Tortoise git tells me that "master equal remotes/origin/master". Branch is set to "master" and upstream is set "remotes/origin/master". So if that is not the right state, which one would be? Sorry, but I'm completely lost here.

[cdunn2001]

Ignore Dani-Hub#1. I don't know why git shows such odd diffs.

If you can find a friend who knows git better, do that. Otherwise, I'll try to resolve the remaining diffs when I have time.