const_json_ptr

[danielaparker]

@BartelNieuwenhuyse wrote,

I think it would be clearer if storage_kind json_const_ref behaves more like a const&

given a has storage_kind json_const_ref

Then this would be ok:
const Json& b = a;

This could make an implicit shallow copy (like I did manually):
Json b = a;

And this would be a compile time error:
Json& b = a;

[danielaparker]

@BartelNieuwenhuyse, Thanks, these comments and your other observations have been very helpful to me. I've found it hard to think about this because basic_json allows storage elements of both a value type and a view type. But unlike std::string/std::string_view, they can't really be separated, much of the benefit comes from adding refs as elements of arrays or values of key-value pairs in objects.

This could make an implicit shallow copy (like I did manually): Json b = a;

In C++ a shallow copy constructor, like the ones the compiler generates automatically, are member-wise copies, which is what we do now. The pointer to the referred value is copied. I would describe what you did manually in merge, sort and sort_by as a partial copy, a copy of the outer container.

My current thinking is that the most C++ thing to do, the behavior that would be least surprising to a user, is to have the basic_json copy constructor do a deep copy. Much as

    std::string_view sv = "foo";
    std::string s = std::string(sv);

makes a copy.

As you noted in your comment on the merge function issue, while deep copy is the simplest (if potentially expensive) solution, we can retain some of the benefits of using refs with a manual solution, and I think we can still do that, by manually making a container that contains refs (using emplace_back(const_json_ptr_arg, &j) rather than push_back(j).)

As well, a copy constructor that performed a deep copy would make the public deep_copy function redundant, which I'd like to dispense with anyway.

And this would be a compile time error:
Json& b = a;

It's an issue that our value-like and view-like classes are the same, in both cases basic_json, so that can't be enforced at compile time.

Somewhat related, if a basic_json holds a const_json_ref, then only the const functions on the basic_json should be called. But if we create one with

json jv(const_json_ptr_arg, &j);

the compiler won't stop us from calling non-const functions on jv, the checks can only be done at run-time. For a few functions that have both const and non-const overloads, particularly array_range() and object_range(), calling those on jv would invoke the wrong ones (the non-const ones), although that would result in a compile time error.

To get around some of these issues, I've experimented with a json_view type (on branch json_view2) that would wrap a ref and expose only const functions. This would allow

json_view<json> jv(j);  // holds a reference to a basic_json j

json ja(json_array_arg);
ja.push_back(jv);  // equivalent to ja.emplace_back(const_json_ptr_arg, &j)

[BartelNieuwenhuyse]

Somewhat related, if a basic_json holds a const_json_ref, then only the const functions on the basic_json should be called. But if we create one with
json jv(const_json_ref_arg, j);
the compiler won't stop us from calling non-const functions on jv, the checks can only be done at run-time. For a few functions that have both const and non-const overloads, particularly array_range() and object_range(), calling those on jv would invoke the wrong ones (the non-const ones)

That is exactly what caused the issues #702 and #705: a non-const json holding a const_json_ref storage kind should not be modified. But this is not enforced at compile time. Instead you get runtime errors.

So my first thought was to have const_json_ref always be const:

const json cjr(json_const_pointer_arg, &j); // would be allowed
json jr(json_const_pointer_arg, &j); // compile error!

But I do not know of a way that this is possible in c++

So as an alternative: why not remove const_json_ref storage kind altogether? If a none-modifiable reference is needed, you can do it in the standard c++ way:

const json cjr(json_pointer_arg, &j); // none-modifiable reference - only const methods can be called compiler checks
json jr(json_pointer_arg, &j); // modifiable reference

But that would not really preserve constness on the referenced json j:

const json cjr(json_pointer_arg, &j); // intention: do not modify j
json jr(cjr); // copies the pointer to j into a none const jr
// I can now modify original j through jr

Which makes me wonder if my solution #703 does not also violates const intent.

I guess at the base, it is a general problem with unions: you only know at runtime which type it will hold. Just as I should not call as_integer() on an object, I should not call none-const methods on a json_const_pointer. But unlike the integer vs. object case where I can check beforehand what it is:

if ( j.is_integer<int>() ) j.as_integer<int>();
else ...

I cannot check on constness.

The most confusing I guess are the functions that have both const and none-const overloads:

json arr(json_array_arg);
json arrCRef(json_const_pointer_arg, &arr);
if (arrCRef.is_array()) // yes, it is an array!
{
    for (const auto& item : arrCRef.array_range()) // runtime error: "Not an array"

Maybe it would already be an improvement to change the error messages with a special case for json_const_pointer storage kind that do point to e.g. an array from 'is not an array', to 'is not modifiable'

[danielaparker]

const json cjr(json_const_pointertr_arg, &j); // would be allowed

There are a couple of issues with that. One, '&j' would be required to be non-const, but in our jmespath implementation, '&j' is a pointer into a const json value supplied by the user. Two, the cases that benefit the most from supporting const refs in json are MultiSelect List and MultiSelect Hash, which require adding const refs into json arrays and objects respectively.

The most confusing I guess are the functions that have both const and none-const overloads

It's certainly an issue, when we don't have a const reference to the json value. In the jmespath implementation, we generally populate the json value with const refs, being careful, and then pass it on as a const reference.

I think the biggest safety issue was having a copy constructor that performed a shallow copy for const (or non-const) refs. As you discovered, even the author of this software could overlook the implications of that in a function implementation. And unlike the issue with const and non-const overloads, this was a runtime issue rather than a compile-time issue. This has been changed to a deep copy. That makes the simple implementation safe, and copy construction can be avoided in special cases where it matters.

We may revisit this in a future release, but for now I think we'll leave it like this.

[danielaparker]

For the longer term it's worth thinking about extracting the "view" parts of the basic_json class into a separate class. A class that can hold a ref, an array of refs, or an object of name/ref pairs. And that implements the const functions of basic_json through delegation.