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

Simple ordered_json that works on all supported compilers #2206

Merged
merged 21 commits into from
Jul 11, 2020

Conversation

gatopeich
Copy link
Contributor

@gatopeich gatopeich commented Jun 21, 2020

Solved the Allocator issue that prevented using a container whose value_type is not the same as std::map's.
The trick was in instantiating AllocatorType with ObjectType::value_type, instead of hard-coding to std::pair<const Key, basic_json>. This it stays the same for std::map but adapts to whatever other container we use in other specializations.


Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

Please don't

[X] checked too 🙂

@gatopeich gatopeich requested a review from nlohmann as a code owner June 21, 2020 21:37
@gatopeich gatopeich marked this pull request as draft June 21, 2020 22:07
@coveralls
Copy link

coveralls commented Jun 21, 2020

Coverage Status

Coverage increased (+0.02%) to 99.981% when pulling 47d154d on gatopeich:issue2179 into 6ee9e5f on nlohmann:issue2179.

gatopeich added 4 commits June 22, 2020 00:03
instead of hard-coding it for std::map's value_type
This is more comprehensive and the "my_workaround_fifo_map" wrapper does not allow to infer value type as required in this branch.

This reverts commit 064a9c9.
@gatopeich
Copy link
Contributor Author

Almost there. Only missing default template arguments for our new ordered_map...

@gatopeich
Copy link
Contributor Author

Looks like GCC 4.9 on Ubuntu 14 cannot handle the partial template definition required for my current solution.
Not sure it is reasonable to try to support such obsolete platform?

@nlohmann
Copy link
Owner

When I look at https://travis-ci.org/github/nlohmann/json/builds/700984944, there seem to be a lot of other compilers complaining. I restarted all failed builds, let's see.

gatopeich added 3 commits June 23, 2020 11:30
to extract the actual ObjectType::value_type
Still fails on older compilers (GCC <= 5.5)
so that it can handle pair<const Key,...>
@gatopeich
Copy link
Contributor Author

Using std::map allocator as a placeholder (acd748e) is the most old-compiler friendly way I could find for the recursive declaration of the Allocator. And it is still not working on some compilers.

Summarizing:

  • Current implementation has object_t::value_type hardcoded to that of std::map: std::pair<const Key, T>, cause AllocatorType is always specialized for that.
  • Providing a non-hardcoded allocator seems impossible for certain compilers without potentially complex refactoring.

So I am trying the next alternative: Implement VecMap:erase that works with the same value_type as std:map...

@nlohmann
Copy link
Owner

Any ideas how proceed here?

@gatopeich
Copy link
Contributor Author

gatopeich commented Jul 2, 2020

I am quite frustrated about how it fails on GCC 4.9 (quite old anyway) and specially XCode 9.3 (not that old but I have no access to such build system).

For the time being we could restrict ordered_json to recent compilers?

Fact is, if we cannot make this simple container work, the claim that users can roll their own seems ambitious... However the same applies: a recent compiler is required.

@gatopeich
Copy link
Contributor Author

Fixed for GCC 5.5, which was giving the same error as v4.9 on travis-ci.
After isolating the issue to one of constructor instantiation, I found a cryptic reference to a retroactive change to the C++11 standard related to issues around constructor inheritance...
So I declared explicit constructors instead, and GCC 5.5 seems happy now.

@gatopeich
Copy link
Contributor Author

Now it works on all environments.
Reviewing the comments and any unintended changes to white space etc...

@gatopeich gatopeich changed the title Clean-up ordered_map declarations Have a simple ordered_map that works on all supported compilers Jul 3, 2020
@gatopeich gatopeich marked this pull request as ready for review July 3, 2020 09:30
@gatopeich gatopeich changed the title Have a simple ordered_map that works on all supported compilers Simple ordered_json that works on all supported compilers Jul 3, 2020
@gatopeich
Copy link
Contributor Author

@nlohmann this is all yours now... 🙂

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments.

README.md Outdated Show resolved Hide resolved
include/nlohmann/json.hpp Outdated Show resolved Hide resolved
include/nlohmann/ordered_map.hpp Outdated Show resolved Hide resolved
include/nlohmann/ordered_map.hpp Outdated Show resolved Hide resolved
include/nlohmann/json.hpp Outdated Show resolved Hide resolved
@gatopeich gatopeich requested a review from nlohmann July 9, 2020 20:14
include/nlohmann/ordered_map.hpp Outdated Show resolved Hide resolved
@gatopeich
Copy link
Contributor Author

Ops! done now.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nlohmann nlohmann merged commit 5f146cb into nlohmann:issue2179 Jul 11, 2020
@nlohmann
Copy link
Owner

Thanks a lot!!!

@nlohmann
Copy link
Owner

I opened a new pull request #2258 to bring the feature to the main branch. Thanks so much for your patience and bringing this forward!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants