Skip to content

Allow hashmap via compilation flag #204

Open
@cdunn2001

Description

@cdunn2001

From anonymous:

Many compilers include hash_map class and now it is even in C++11 standard by the name unordered_map.
I propose another macro JSON_MAP_TYPE which will be defined to the best possible class.
Currently I can choose only JSON_USE_CPPTL_SMALLMAP or not.
It is not enough, I want to use map based on hash, it will be more efficient.

Activity

Dani-Hub

Dani-Hub commented on Mar 7, 2015

@Dani-Hub
Contributor

Instead of introducing just another flag it seems to me preferable to test the compiler macro __cplusplus against the value that refers to (at least) C++11. Visual Studio compilers - even though they provide std::unordered_map - can be tested against the value of _MSC_VER.

The following preprocessor test presents a possible extension of the current one:

#ifndef JSON_USE_CPPTL_SMALLMAP
#  if (__cplusplus >= 201103L) || (defined(_MSC_VER) && _MSC_VER >= 1700)
#    include <unordered_map>
#  else
#    include <map>
#  endif
#else
#include <cpptl/smallmap.h>
#endif

If interest exist to follow this direction I could try to provide a patch file.

cdunn2001

cdunn2001 commented on Mar 7, 2015

@cdunn2001
ContributorAuthor

That sounds like a good idea in general.

However, we have also to worry about binary compatibility. If it's a flag, people can use it at their leisure. Otherwise, we have to be very careful about the sizes of data-structures. (It's not the map; it's the map iterators which we expose.)

Dani-Hub

Dani-Hub commented on Mar 8, 2015

@Dani-Hub
Contributor

In this case I have a slightly different suggestion: For the C++11 development branch (1.5.x) of jsoncpp, I would suggest to replace unconditionally the reference to <map> by <unordered_map>.

cdunn2001

cdunn2001 commented on Mar 8, 2015

@cdunn2001
ContributorAuthor

ObjectValues::Iterator is a member of ValueIteratorBase. (I'm not the fond of those iterators, but they exist.) Maybe it's not really a big deal to break binary-compatibility for the 1.y.z series, but let's see a benefit first. What is the improvement in runtime and memory?

Dani-Hub

Dani-Hub commented on Mar 8, 2015

@Dani-Hub
Contributor

A std::map will provide logarithmic complexities for it's operations, while std::unordered_map in the average realizes constant complexity.

cdunn2001

cdunn2001 commented on Mar 9, 2015

@cdunn2001
ContributorAuthor

Hmmm. I really hate breaking binary-compatibility. People definitely instantiate Iterators in for-loops. It's hard to justify without a major-version bump. If we could say, "With this input, the savings are such and such," at least we would have an argument.

On the other hand, a major-version bump is not terrible. But maybe we should do more things along with this. E.g., the bump is the opportunity to drop the deprecated classes. So I'd rather not rush into this.

But I'm fine with a compiler-flag.

MartinBonner

MartinBonner commented on Nov 18, 2015

@MartinBonner

Binary compatibility: Also remember that at the moment maps return their members in alphabetical order. With a change to unordered_map, they wouldn't. Now obviously, JsonCpp doesn't promise to return members in order - but I bet that there is someone, somewhere, whose application will be broken if it stops doing so.

Which sounds to me like a very strong argument for a major version bump or a compiler flag.

Edit: Fix typo

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Allow hashmap via compilation flag · Issue #204 · open-source-parsers/jsoncpp