Redo high-level ownership strategy

[jfirebaugh]

The previous use of PointerToValue was unsafe. Consider the following code:

jni::UniqueLocalObject<Foo> f_ = ...;
const Object<Foo>& f = *f_;

Now f is a dangling reference. Why?

• std::unique_ptr::operator* is defined as *get().
• get()'s return type is pointer, which for UniqueLocalObject (and friends) is PointerToValue. This means that get copies PointerToValue -- in the case of *get(), that's a copy into a temporary.
• Copying PointerToValue copies the held value.
• Then the copy is dereferenced (*get()), producing a reference to the copy of the held value.
• operator* returns. The PointerToValue temporary copy is destroyed, and the returned reference is now dangling.

AFAICT, there's no way to safely do what we're trying to do with PointerToValue and unique_ptr, so this replaces unique_ptr with a special RAII type that has the exact semantics we need.

(Another thing that doesn't work is using Object<> as the pointer type. That doesn't work because std::unique_ptr::operator-> returns pointer, and we want it to return Object<>*, so that p->Get(...) and so on work.)

Note that this is a breaking change, because e.g. the return type of release() is now one of the high-level types (Object<>, Array<>, Class<>), not PointerToValue<>. You can see the typical change required in the hello.cpp example code.

[kkaefer]

While reviewing this, I also noticed that our *Deleter functions can be constructed with the default constructor, which means that JNIEnv* will be null. Can we disallow creating without JNIEnv*?

It'd also be cool to see a test that confirms that the new behavior points to the correct jni::Object.

[kkaefer]

Why do we need this overload?

[jfirebaugh]

It permits construction of UniquePointerlike<Base> from UniquePointerlike<Derived> when Derived converts to Base. See #25/#26.

[kkaefer]

Ah cool; can we capture that in an enable_if?

[kkaefer]

Are we intentionally copying the object here instead of moving it? Can we make sure that a move-only type can be used as well?

[kkaefer]

This assumes that the contained type is default-constructible. unique_resource is essentially the same as this except without this caveat.

[kkaefer]

😨 Why are we casting away const here?

[jfirebaugh]

I was thinking that UniquePointerlike::operator* should have pointer-like semantics even though the "pointed to" value is held as member data. I.e. be able to dereference a const pointer and get a non-const reference. Compare std::unique_ptr<T>::operator*, which is const but returns non-const T&.

I'm not sure this is correct though. The problem is that Object<>, Array<>, and Class<> have assignment operators, which effectively allow you to change what a const UniquePointerlike points to. That's dangerous.

We could have operator* return a const T&, but then you wouldn't be able to use Array<>::Set and Array<>::SetRegion.

We could make Array<>::Set and Array<>::SetRegion const, but that doesn't seem right. Shouldn't collection types always propagate const?

We could make UniquePointerlike a const-propagating type, providing const T& operator*() const and T& operator*(). But it seems to me that this doesn't really fix the problem -- we want reset() or move assignment to be the only way of changing what even a non-const UniquePointerlike points to.

We could abandon UniquePointerlike entirely and instead define a family of types:

• LocalObject<>
• LocalString
• LocalArray<>
• LocalClass<>
• GlobalObject<>
• GlobalString
• GlobalArray<>
• GlobalClass<>
• WeakObject<>
• WeakString
• WeakArray<>
• WeakClass<>

Each of these would have the same member functions as the existing un-prefixed types, except that they would all be move only and their destructor would release in the appropriate way.

Thoughts?

[jfirebaugh]

More options:

• Have operator* return T (by value copy). This wouldn't work for operator-> though; it's the same issue as using Object<> as unique_ptr's pointer type: you wouldn't be able to do p->Get(...).

• Recapitulate the type punning we're using for the low level wrappers. Instead of wrapping jobject* in Object<>, cast jobject* to Object<>*. Then we can continue to use unique_ptr with custom deleters. Since Object<> et al have no data (other than the pointer) and no vtable, I think this is safe, though unorthodox.

  For derived-to-base conversion to continue to work in this approach, Object<DerivedTag> would need to C++-inherit from Object<BaseTag>.

[jfirebaugh]

• Delete Object<>::operator= ???

[kkaefer]

Compare std::unique_ptr<T>::operator*, which is const but returns non-const T&.

That's not a apt comparison: the constness of that operator refers to the fact that the container can't be changed (i.e. the pointer it holds cannot be replaced). A container like std::unique_ptr<const T> would still return a const T& with that operator.

Thinking about this more, I think it makes sense to implement the approach #32 suggests in combination with creating a family of holder types.

[jfirebaugh]

My point was that unique_ptr does not propagate const-ness, and in general, pointer types don't propagate const-ness. You can mutate the pointed-to object through a const pointer; to make the pointed-to object itself const requires an additional const in a different place (unique_ptr<const T>, as you say, or const T const *).

I wound up fixing the hole by deleting operator=s and friending UniquePointerlike so that only it can update the pointer. I don't think reassignability was much used for Object/Class/Array, so this feels like a better solution than duplicating member functions into parallel class families, and then dealing with needing conversions between them. It's also more backwards compatible, as it doesn't require a lot of rewriting -> to . in client code.

[kkaefer]

This requires the contained type to be castable to boolean. Is that what we want?

[jfirebaugh]

Yes -- UniquePointerlike is intended to be used only with Object<>, Array<>, and Class<>, not as a general-purpose smart pointer.

[jfirebaugh]

Related: #32.