Json::Value constructor: Accept any arithmetic type.

[BillyDonahue]

Allow construction of Json::Value from size_t, short, etc.
Fixes issue #64

[jacobsa]

Mostly out of curiosity: why this is_specialized here? According to cppreference, is_integer is false for non-specialized types. Maybe that's not guaranteed for future versions of the standard?

[jacobsa]

I'll take this. Left some comments. (Thanks for improving the tests, by the way.)

[BillyDonahue]

@jacobsa done. PTAL. thx.

[jacobsa]

I think this should be on line 102 below. The stuff before then is just a straightforward use of pre-C++11 <limits>.

[BillyDonahue]

Well, yes and no. If I were in C++11, I'd just use std::enable_if, std::is_arithmetic, std::is_integral, and std::is_floating_point. That is, even the IsIntegral and IsFloatingPoint would go away.

[jacobsa]

Ah, I thought it was just std::enable_if. Right, no problem then.

[cdunn2001]

I've inserted orodbhen's code for historical reference. The final result is from BillyDonahue.

Now, I do not like that complexity. I'd like to offer an alternative approach. I prefer this:

Json::Value Json::RealValue(double v) {
    /*construct real_ */
    return Json::Value(... whatever ...);
}
Json::Value Json::SignedValue(Int64 v) {
    /* construct int_ */
}
Json::Value Json::UnsignedValue(UInt64 v) {
    /* construct uint_ */
}

Those might be called factories, but they act like simple sub-types (which is why I use UpperCase). They're easy to use, and we could deprecate (but not delete) the old ctors. (The fact that some type-conversions fail to compile with the old ctors is a feature, not a bug.)

If you like that idea, then I would take it a step further and provide a factory for each integral type. I don't see why people can't be explicit. Then, we could provide another factory which uses a template and all of Billy's wizardry, and that templated version could be #included optionally, from a separate header file.

// json/value_templates.h
namespace Json {

template <typename T>
Value NumericValue(T v) {
    // ...
}

// SFINAE magic ...
}

What do you think?

[jacobsa]

@cdunn2001: Sorry, could you expand on which types don't compile and why that's a feature? Is it the issue I mention below, or something more?

@BillyDonahue: It occurs to me that this change causes a subtle issue with type-narrowing. Previously if jsoncpp was configured without int64 support, then I think the compiler would tell you off for doing this:

int64 foo = ...;
Json::Value v{ foo };  // Error, type narrowing

Now I believe this will not give a compiler error, but will silently truncate (or worse) the input value in the constructor. Maybe we should limit this to numeric types that will fit?

[BillyDonahue]

@jacobsa , you raise an interesting point, as true 64-bit numbers are on thin ice here. JavaScript 'number' variables are IEEE-754 doubles with 53-bit mantissa and can't hold them exactly. I didn't realize support for int64 was optional. What does jsoncpp currently emit when given an int64 that can't fit into a double?

The most important type that doesn't compile is Value(std::size_t). When std::size_t is defined to be unsigned long (as in gcc-ia64-linux), it outranks int and can't be promoted to it. So it must be converted. It can convert to any of the double, int, long long, unsigned, or unsigned long long constructors, and so it's ambiguous.

You don't like the constrained template technique. Ok.
An easier fix for this problem (which I just can't consider a feature) might be to define the missing 'long' and 'unsigned long' constructors. You need to support those separately because they outrank int in the integral promotions, but aren't long long (aka Json::Int64). My feeling is that we're getting integer conversions whether we want them or not, so we might as well do them consistently.

@cdunn2001 , As a user, I think encoding prefixes to function names to nail down their argument types adds caller-side complexity in practice. The argument types are what they are, and as a user I expect a library to produce appropriate parameter types from my supplied arguments. Callers can't be explicit in deduced contexts (when using auto or in templates), it's just so much easier to have one name for a function, and not have to reinvent this traits-based dispatch to differently-named functions at call sites. Such auto callers should, if we do things properly, write to interfaces and let the overload set DWIM with some wiggle room. That is, they should be able to write code with the intent: "I don't care what kind of numbers this vector holds, just put its elements into a Json Value." The code to make that work properly in deduced contexts is noisy and worse, distributed across callers.

Reducing the constructor set and relying more on factories might make more sense if Value were cheaply movable, but it isn't. I think sticking with constructors is okay here. If you want to make those converting constructors explicit, maybe that eliminates some of your concerns? It's still a compatibility problem, though, so you probably can't change it now without cutting a major revision and breaking backward compatibility.

[jacobsa]

I don't mind your template at all; in fact, I prefer it over stuttering. But:

• We definitely don't want any narrowing/overflow-related undefined behavior.
• Ideally the compiler would tell the user off if they are narrowing, instead of us silently truncating.

What do you think of the idea of doing enable_if on integral types that are no larger than LargestInt/LargestUInt? For that matter, can we just have the constructor accept those directly, or do we get into the size_t problem then? What do we want size_t to do?

Regarding 64-bit support, it's a mess. Search around for JSON_HAS_INT64 to see. I'm inclined to abandon support for ancient compilers by killing off that macro in a separate pull request. I would even consider using intmax_t and uintmax_t as the representation, and killing off the idea of "largest" altogether, leaving that to the compiler to decide. What do you think?

Note that, as I understand it, JSON places no size or representability restrictions on numbers; it's only JavaScript implementations that do. I would prefer to keep the library 64-bit clean (in the sense that all representable 64-bit integers round trip exactly) if possible.

I agree about using constructors rather than factories.

[BillyDonahue]

What do you think of the idea of doing enable_if on integral types that are no larger than LargestInt/LargestUInt? For that matter, can we just have the constructor accept those directly, or do we get into the size_t problem then?

We'd have the size_t problem if we leave out any of the integral types that outrank int. Everything that doesn't outrank int will be promoted to int or unsigned. IIUC, the following ordinary overloads should be enough to cover all arithmetic types:

// int and smaller-ranked integrals are covered by int and unsigned.
Value(int);
Value(unsigned);

// integrals that outrank int and unsigned.
Value(long);
Value(long long);
Value(unsigned long);
Value(unsigned long long);

// all floats come here
Value(double);
Value(long double);  // ???

What do we want size_t to do?

I'd assume we want Value(size_t x) to yield type_ = uintType and value_.uint_ = x.

JSON places no size or representability restrictions on numbers

Quite right. I was given some bad information. Sorry about that. Always read the spec!

[cdunn2001]

@jacobsa, Someone mentioned a compilation problem in a previous Issue. I thought this whole Issue was an alternative to that implementation. See #64 and #65, I think. I did not look at it closely.

@BillyDonahue, I don't want to argue, except to say that I find go-lang refreshing in its disregard for the supposed necessity of generics. In particular, I don't agree that anybody needs a vector of disparate types. But really, I respect your opinion on this. I just thought I'd throw out my idea and see if anybody liked it.

I am glad that you both have finally accepted that the maximum precision depends not on type but on context (e.g. Javascript) which is why I would store the numeric values as strings and convert with a conversion-layer tailored to the application, which I why I believe you are spending too much effort on type-conversion. You will never have a perfect API for numeric conversion. You cannot satisfy everyone.

As for the efficiency of returning a Json::Value object, I was only offering the basic idea for discussion. In practice, I would follow the Google C++ Style Guide and let the caller allocate the memory.

void RealValue(double x, Json::Value* result);

Alternatively, I would let the caller inject an Allocator, and I would return a pointer. Or, I would just ignore efficiency, as that is not the primary goal of jsoncpp.

And that's my main point: I don't think we can make jsoncpp the fastest library, given its goals. But I think we can make it the most useful.

With that in mind, how about this:

Value::Value(RealValue v);

RealValue can be an actual a type.

Anyway, I won't object to whatever you both agree on. You are definitely keeping with the spirit of the current jsoncpp API.

[jacobsa]

Arbitrary precision is a horse of a different color. Maybe not a terrible idea, but only as an opt-in feature and maybe after some other baby steps first. :-)

I apologize that this review has gone far afield, but I guess these issues are tied up in the answer. Here's my concrete proposal:

1. Remove the JSON_HAS_INT64 checks, in favor of the true branch in each case. That is, we always assume the presence of 64-bit integers. Those using a ridiculously old or incomplete C++ implementation can use an old version of the library, or maintain their own fork and back-port changes as necessary. (I feel the same way about C++11 if you are interested in taking that on, but we can leave that for another discussion unless doing it first makes the rest of this easier.)
2. I believe a side effect of step 1 is that the representation for integers becomes int64_t/uint64_t. Consider taking this further, making it intmax_t/uintmax_t. Now we can represent any integer that the C++ implementation can. (And also perhaps we get back support for systems where the maximum integer size is 32 bits and intmax_t is present, after we broke them in step 1? I'm not sure there are any such systems of note.)
3. Now I believe it is safe to take any integral type in the constructor. Equivalently, we can simply have one intmax_t constructor and one uintmax_t constructor.

@BillyDonahue: How do you like this style?
@cdunn2001: Do you see anything that is bad enough to be a deal-breaker here?

[BillyDonahue]

1. Remove the JSON_HAS_INT64 checks, in favor of the true branch in each case. That is, we always assume the presence of 64-bit integers. Those using a ridiculously old or incomplete C++ implementation can use an old version of the library, or maintain their own fork and back-port changes as necessary. (I feel the same way about C++11 if you are interested in taking that on, but we can leave that for another discussion unless doing it first makes the rest of this easier.)

This is to eliminate your concern about narrowing conversions, right?
Sounds right.

1. I believe a side effect of step 1 is that the representation for integers becomes int64_t/uint64_t. Consider taking this further, making it intmax_t/uintmax_t. Now we can represent any integer that the C++ implementation can. (And also perhaps we get back support for systems where the maximum integer size is 32 bits and intmax_t is present, after we broke them in step 1? I'm not sure there are any such systems of note.)

The existence of at least one 64-bit integral type doesn't imply the definition of int64_t as an alias for one of them. int64_t is a C99ism. So this isn't a side effect of step 1, really, but is itself a further step. I'm thinking of the MSVCism of __int64 that's in config.h now.

1. Now I believe it is safe to take any integral type in the constructor.
   Equivalently, we can simply have one intmax_t constructor and one uintmax_t constructor.

I don't think it's simple. If you did that, an 'int' argument would be ambiguous, for example.
You can experiment with it here: http://goo.gl/zctPC9

[jacobsa]

Sorry yes, I realized my understanding of type conversions was wrong yet again while in the shower. So modify that last step to be your templated constructor.

Is the MSVC __int64 thing current? Does MSVC seriously not support C99?

[BillyGoto]

MSVC commitment to C99 is very recent. It's something of a scandal. int64_t
was only introduced in MSVC2010.

I agree with your plan, Aaron. We could also eliminate narrowing if we
wanted to, with another enable_if trait, I guess. I'm too tired to do it
now, but it's an idea.

On Mon, Nov 10, 2014 at 2:35 PM, Aaron Jacobs [email protected]
wrote:

Sorry yes, I realized my understanding of type conversions was wrong yet
again while in the shower. So modify that last step to be your templated
constructor.

Is the MSVC __int64 thing current? Does MSVC seriously not support C99?

—
Reply to this email directly or view it on GitHub
#66 (comment)
.

ǝnɥɐuop ʎllıq

[cdunn2001]

@cdunn2001: Do you see anything that is bad enough to be a deal-breaker here?

No.

[cdunn2001]

Since the main branch is strictly C++11 now, will this solution work? What about MSVC?

[BillyDonahue]

In C++11 a much more elegant version of this code would be possible.
So I'd rewrite it for the main branch using the C++11 common subset supported by MSVC.
How far back into MSVC do you need to support? Their C++11 compliance is a moving target.

[cdunn2001]

2013 at the earliest. I'm even fine with 2015. Folks with a problem can always use an older release.

At some point, you might have to break some API-compatibility. In that case, put it on a 2.y.z branch. I expect most C++11 changes will break binary compatibility anyway.

Hmmm. I really need to get the hidden implementation done quickly, so it will not conflict with other new work. But just do what you think best.

[hjmjohnson]

@BillyDonahue I'm closing this 4 year old PR. I think that the move to C++11 makes this PR not consistent with the current code base.

If there are still changes that are needed, please feel free to re-open the PR (preferably rebased on top of the current master branch).