Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support gcc 4.8.1 #212

Closed
wants to merge 3 commits into from
Closed

support gcc 4.8.1 #212

wants to merge 3 commits into from

Conversation

abc100m
Copy link

@abc100m abc100m commented Feb 20, 2016

for issue #211 , support gcc 4.8.1 since it already had fully support c++ 11 (though STL library are not yet)

@nlohmann
Copy link
Owner

Hi @abc100m, thanks for the PR. Could you please edit file json.hpp.re2c, because json.hpp is generated from this file. Does the PR also cope with user-defined string literals?

@nlohmann
Copy link
Owner

Hi @abc100m, once you edited both files, could you also run the tests to make sure every feature works.

@abc100m
Copy link
Author

abc100m commented Feb 22, 2016

Hi, I had edited json.hpp.re2c and had tested it on my computer like this way:

>cd src
>rm json.hpp
>touch json.hpp
>re2c json.hpp.re2c > json.hpp
> cd ..
> make
>./json_unit
>./json_unit "*"

it works and all the test case were passed.

for bug 57824 of gcc 4.8, I think it may be is the compiler issue, and I have no idea how to fix it. I'm just going to not use the "user-defined string" feature.

thank you for your powerfully but simple json library, it's really great!

@nlohmann
Copy link
Owner

Thanks for the update. I haven't found the time to check it though :-(

Thanks for your patience!

@nlohmann
Copy link
Owner

nlohmann commented Mar 6, 2016

I tried to compile the code, but got several errors:

$ g++-4.8 --version
g++-4.8 (Homebrew gcc48 4.8.5) 4.8.5
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ make CXX=g++-4.8
g++-4.8 -std=c++11  -Wall -Wextra -pedantic -Weffc++ -Wcast-align -Wcast-qual -Wctor-dtor-privacy -Wdisabled-optimization -Wformat=2 -Winit-self -Wmissing-declarations -Wmissing-include-dirs -Wold-style-cast -Woverloaded-virtual -Wredundant-decls -Wshadow -Wsign-conversion -Wsign-promo -Wstrict-overflow=5 -Wswitch -Wundef -Wno-unused -Wnon-virtual-dtor -Wreorder -Wdeprecated -Wfloat-equal  -I src -I test test/unit.cpp  -o json_unit
test/unit.cpp:11899:32: error: unterminated raw string
             CHECK_NOTHROW(json(R"(
                                ^
test/unit.cpp:11914:14: warning: missing terminating " character [enabled by default]
             )"));
              ^
test/unit.cpp:11918:32: error: unterminated raw string
             CHECK_NOTHROW(json(R"(
                                ^
test/unit.cpp:11940:15: warning: missing terminating " character [enabled by default]
             ])"));
               ^
test/unit.cpp:12105:30: error: unterminated raw string
         const auto content = R"(
                              ^
test/unit.cpp:12110:11: warning: missing terminating " character [enabled by default]
         })";
           ^
test/unit.cpp:12349:0: error: unterminated argument list invoking macro "CHECK_NOTHROW"

 ^
In file included from test/unit.cpp:26:0:
src/json.hpp: In instantiation of 'class nlohmann::basic_json<>::const_iterator':
src/json.hpp:6834:11:   required from 'class nlohmann::basic_json<>::iterator'
test/unit.cpp:953:24:   required from here
src/json.hpp:6290:11: warning: base class 'struct std::iterator<std::random_access_iterator_tag, const nlohmann::basic_json<>, long int, const nlohmann::basic_json<>*, const nlohmann::basic_json<>&>' has a non-virtual destructor [-Weffc++]
     class const_iterator : public std::iterator<std::random_access_iterator_tag, const basic_json>
           ^
src/json.hpp: In instantiation of 'class nlohmann::basic_json<>::iterator':
test/unit.cpp:953:24:   required from here
src/json.hpp:6834:11: warning: base class 'class nlohmann::basic_json<>::const_iterator' has a non-virtual destructor [-Weffc++]
     class iterator : public const_iterator
           ^
src/json.hpp: In instantiation of 'class nlohmann::basic_json<>::json_reverse_iterator<nlohmann::basic_json<>::iterator>':
test/unit.cpp:4987:40:   required from here
src/json.hpp:6975:11: warning: base class 'class std::reverse_iterator<nlohmann::basic_json<>::iterator>' has a non-virtual destructor [-Weffc++]
     class json_reverse_iterator : public std::reverse_iterator<Base>
           ^
src/json.hpp: In instantiation of 'class nlohmann::basic_json<>::json_reverse_iterator<nlohmann::basic_json<>::const_iterator>':
test/unit.cpp:5012:46:   required from here
src/json.hpp:6975:11: warning: base class 'class std::reverse_iterator<nlohmann::basic_json<>::const_iterator>' has a non-virtual destructor [-Weffc++]
test/unit.cpp: In function 'void ____C_A_T_C_H____T_E_S_T____11880()':
test/unit.cpp:11899:13: error: 'CHECK_NOTHROW' was not declared in this scope
             CHECK_NOTHROW(json(R"(
             ^
test/unit.cpp:11899:13: error: expected ';' at end of input
test/unit.cpp:11899:13: error: expected '}' at end of input
test/unit.cpp:11899:13: error: expected '}' at end of input
test/unit.cpp:11899:13: error: expected '}' at end of input
make: *** [json_unit] Error 1

@abc100m
Copy link
Author

abc100m commented Mar 11, 2016

well, bug 57824 of gcc 4.8.1 maybe can't be fixed.

this PR only fix bug of STL library , and if we don't use C++11 raw strings literals, that will be fine.

@nlohmann
Copy link
Owner

Sorry, but this is not helping. There is no way GCC 4.8 will ever support the library, and such workarounds only make the library harder to maintain. I shall add a link to this PR to the README file to document a solution for the STL issue.

Thanks for the effort anyway!

@nlohmann nlohmann closed this Mar 30, 2016
@nlohmann nlohmann added solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope) and removed kind: enhancement/improvement labels Mar 30, 2016
nlohmann added a commit that referenced this pull request Mar 30, 2016
guillaumeblanc added a commit to guillaumeblanc/ozz-animation that referenced this pull request Aug 24, 2017
@nlohmann nlohmann mentioned this pull request May 3, 2018
pb-dseifert added a commit to PacificBiosciences/pbcopper that referenced this pull request Jun 7, 2018
* Also apply nlohmann/json#212
  for GCC 4.8 support for bioconda.
@henryiii
Copy link
Contributor

henryiii commented Jun 15, 2018

Since people are manually applying this patch, wouldn't it make sense to add this to master for now? It's not an awful maintenance burden from the look of it. I'm working with CUDA on CentOS 7, and that's all built on top of CentOS 7's GCC 4.8 stack. CentOS/RHEL 7 is the newest version available and is built on GCC 4.8, FYI. At least until Red Hat releases a successor to RHEL 7.

The dual form in this patch of insert would not be needed; you could just use the 3 line version instead of the 1 line version.

This could be simplified significantly by adding a little inline function that wraps iterator, then returns the correct result. You could optionally toggle between the 3 line and 1 line version inside that function, or you could just provide the 3 line version.

@henryiii
Copy link
Contributor

henryiii commented Jun 15, 2018

This is what I'm thinking:

@@ -123,7 +123,7 @@ using json = basic_json<>;
         #error "unsupported Clang version - see https://github.com/nlohmann/json#supported-compilers"
     #endif
 #elif defined(__GNUC__) && !(defined(__ICC) || defined(__INTEL_COMPILER))
-    #if (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__) < 40900
+    #if (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__) < 40800
         #error "unsupported GCC version - see https://github.com/nlohmann/json#supported-compilers"
     #endif
 #endif
@@ -10435,6 +10435,23 @@ class basic_json
         return object.release();
     }

+    /// helper for insertion of an iterator (supports GCC 4.8+)
+    template<typename... Args>
+    iterator insert_iterator(const_iterator pos, Args&& ... args)
+    {
+        iterator result(this);
+        assert(m_value.array != nullptr);
+
+        auto insert_pos = std::distance(m_value.array->begin(), pos.m_it.array_iterator);
+        m_value.array->insert(pos.m_it.array_iterator, std::forward<Args>(args)...);
+        result.m_it.array_iterator = m_value.array->begin() + insert_pos;
+
+        // For GCC 4.9+ only, this could become:
+        // result.m_it.array_iterator = m_value.array->insert(pos.m_it.array_iterator, cnt, val);
+
+        return result;
+    }
+
     ////////////////////////
     // JSON value storage //
     ////////////////////////
@@ -14579,9 +14596,7 @@ class basic_json
             }

             // insert to array and return iterator
-            iterator result(this);
-            result.m_it.array_iterator = m_value.array->insert(pos.m_it.array_iterator, val);
-            return result;
+            return insert_iterator(pos, val);
         }

         JSON_THROW(type_error::create(309, "cannot use insert() with " + std::string(type_name())));
@@ -14632,9 +14647,7 @@ class basic_json
             }

             // insert to array and return iterator
-            iterator result(this);
-            result.m_it.array_iterator = m_value.array->insert(pos.m_it.array_iterator, cnt, val);
-            return result;
+            return insert_iterator(pos, cnt, val);
         }

         JSON_THROW(type_error::create(309, "cannot use insert() with " + std::string(type_name())));
@@ -14696,12 +14709,10 @@ class basic_json
         }

         // insert to array and return iterator
-        iterator result(this);
-        result.m_it.array_iterator = m_value.array->insert(
-                                         pos.m_it.array_iterator,
-                                         first.m_it.array_iterator,
-                                         last.m_it.array_iterator);
-        return result;
+        return insert_iterator(
+                               pos,
+                               first.m_it.array_iterator,
+                               last.m_it.array_iterator);
     }

     /*!
@@ -14743,9 +14754,7 @@ class basic_json
         }

         // insert to array and return iterator
-        iterator result(this);
-        result.m_it.array_iterator = m_value.array->insert(pos.m_it.array_iterator, ilist.begin(), ilist.end());
-        return result;
+        return insert_iterator(pos, ilist.begin(), ilist.end());
     }

     /*!

That would reduce maintenance burden by factoring out similar code into one place, and would also support GCC 4.8 partially. It could also be done with the #if.

(I've left in the old assert, but it seems to not be in the current .hpp, so you could remove it. It's just in one place now instead of three.)

@henryiii
Copy link
Contributor

By the way, by "partial support", I just mean a user can't use raw strings in macros. That's already a requirement for any user using GCC 4.8 and is not a fault or failure of this library.

My solution is actually fewer lines of code than the current version. ;)

@nlohmann
Copy link
Owner

I understand your efforts, but GCC 4.8 will never fully support this library and your fixes seem to only enable a few use cases.

hernando pushed a commit to BlueBrain/Brion that referenced this pull request Jun 27, 2018
@cjh1
Copy link
Contributor

cjh1 commented Sep 20, 2018

@henryiii Do you by any chance have your patch ported to the latest version?

cjh1 added a commit to cjh1/avogadrolibs that referenced this pull request Sep 20, 2018
@henryiii
Copy link
Contributor

No I haven't updated it yet. That's great. Would your patch be accepted as a PR, @nlohmann? This doesn't add much as far as complexity for maintenance. (and is currently having to be maintained separately by all the people running CentOS/RHEL 7.

@nlohmann
Copy link
Owner

It depends on the PR :)

henryiii pushed a commit to henryiii/json that referenced this pull request Sep 25, 2018
henryiii pushed a commit to henryiii/json that referenced this pull request Sep 25, 2018
henryiii pushed a commit to henryiii/json that referenced this pull request Sep 25, 2018
@henryiii henryiii mentioned this pull request Sep 25, 2018
@henryiii
Copy link
Contributor

@nlohmann I've added a PR with a gcc 4.8 Travis test and all tests fully pass. No #if's added anywhere.

cjh1 added a commit to cjh1/avogadrolibs that referenced this pull request Sep 27, 2018
@henryiii
Copy link
Contributor

For those watching this thread and have their own patches: develop now supports GCC 4.8 and looks like 3.2.1 should have it!

@cjh1
Copy link
Contributor

cjh1 commented Sep 28, 2018

@henryiii Thanks for getting this in!

cjh1 added a commit to cjh1/avogadrolibs that referenced this pull request Oct 3, 2018
See nlohmann/json#212 for details

Signed-off-by: Chris Harris <chris.harris@kitware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants