Add explicitUpcast

[HGuillemet]

This PR addresses issue #651.
A first attempt to solve it was PR #655.
But I find the solution brought by this PR better because:

• it introduces much less changes in the Parser
• the new code mostly runs when the new Info explicitUpcast is activated, so there is little chance of breaking anything
• we keep the Java inheritance relationship

How it works:

• new Info explicitUpcast is used to mark those classes that need a static_cast for converting a pointer of a subclass to a pointer of this class.
• Cast asB() methods are added for all marked classes B and all their direct subclasses.
• wrapper are created for all instance native methods of marked class and all native methods taking an instance of marked classes as an argument. For instance:

class B {
  public void f() { asB()._f(); }
  private native @Name("f") void _f();
  
  public B asB() { return this }
}

class D extends B {
  @Override public B asB() {  return asB(this); }
  public static native @Name("static_cast<B*>") B asB(B b);
}

class X {
  static public void g(B b) { _g(b.asB()); }
  static private native @Name("g") void _g(B b);
}

• Since the native method doesn't have its original name (_f instead of f), in order to still support virtualization, we add an optional parameter javaName to annotation @Virtual that tells the generator the name of the java callback method (f in this example). This optional parameter is added for all virtualized wrapped method.
• we trace polymorphic classes and virtual inheritance, in order to raise a warning (could be an error) if virtual inheritance to a polymorphic class is detected and if the parent polymorphic class doesn't have explicitUpcast. This feature could be removed if you prefer to minimize the code changes.

asX explicit casts were already used to map multiple inheritance. This PR proposes a version of asX that can takes care of smart pointers (could be in a separate PR), by using adapters and replacing the static_cast by a static_pointer_cast. However this still need improvements because:

1. it only supports shared_ptr
2. Without a specific info to mark classes managed by smart pointers (as initially proposed in PR Add share Info #668) we have no easy way to detect when to generate the adapter-version of this method. Current non-satisfactory way is to test for the presence of an annotation on the constructor of one of the class. Do you have a better idea ?

Finally, to improve readability and avoid code repetitions I introduced method removeAnnotations. This yields many line changes but no functional changes.

I'll compare parsing result of existing presets to detect breakages if this PR is to be merged.

[saudet]

This looks like something reasonable, although instead of trying to work on unrelated changes all at once, let's at least separate all that into a couple of independent commits.

• Since the native method doesn't have its original name (_f instead of f), in order to still support virtualization, we add an optional parameter javaName to annotation @Virtual that tells the generator the name of the java callback method (f in this example). This optional parameter is added for all virtualized wrapped method.

There must be a cleaner way of doing this...

asX explicit casts were already used to map multiple inheritance. This PR proposes a version of asX that can takes care of smart pointers (could be in a separate PR), by using adapters and replacing the static_cast by a static_pointer_cast. However this still need improvements because:

1. it only supports shared_ptr

2. Without a specific info to mark classes managed by smart pointers (as initially proposed in PR Add share Info #668) we have no easy way to detect when to generate the adapter-version of this method. Current non-satisfactory way is to test for the presence of an annotation on the constructor of one of the class. Do you have a better idea ?

I'm not sure what you're asking here. Functions like static_pointer_cast return a new shared_ptr, but we don't want to do that here anyway. Simply casting the pointer is sufficient. If you want to create a new shared_ptr though, we can still use SharedPtrAdapter for that, just like you did in pull #668.

I'll compare parsing result of existing presets to detect breakages if this PR is to be merged.

Please do!

[HGuillemet]

• Since the native method doesn't have its original name (_f instead of f), in order to still support virtualization, we add an optional parameter javaName to annotation @Virtual that tells the generator the name of the java callback method (f in this example). This optional parameter is added for all virtualized wrapped method.

There must be a cleaner way of doing this...

Any idea ? Unless we accept of not making the thing transparent and ask the user to use the wrapped name of the method (_f or other) when overriding in Java, I don't see another way. This new option adds just one line of code in Generator.

asX explicit casts were already used to map multiple inheritance. This PR proposes a version of asX that can takes care of smart pointers (could be in a separate PR), by using adapters and replacing the static_cast by a static_pointer_cast. However this still need improvements because:

1. it only supports shared_ptr
2. Without a specific info to mark classes managed by smart pointers (as initially proposed in PR Add share Info #668) we have no easy way to detect when to generate the adapter-version of this method. Current non-satisfactory way is to test for the presence of an annotation on the constructor of one of the class. Do you have a better idea ?

I'm not sure what you're asking here. Functions like static_pointer_cast return a new shared_ptr, but we don't want to do that here anyway. Simply casting the pointer is sufficient. If you want to create a new shared_ptr though, we can still use SharedPtrAdapter for that, just like you did in pull #668.

static_pointer_cast returns a new shared pointer to the same pointer, increasing the use count. That's what we need.
If we simply cast the pointer, we will create a java object without a shared pointer or, if we use the adapter, an object containing a new shared pointer but with a reinitialised use count. In both case we will end up with a segmentation fault.
For unique and weak pointers I guess normal cast is fine, but I'm unsure.

I can move this part to another PR if you wish, but then the explicitUpcast feature would be incompatible with the use of shared pointers.

[saudet]

If we simply cast the pointer, we will create a java object without a shared pointer or, if we use the adapter, an object containing a new shared pointer but with a reinitialised use count. In both case we will end up with a segmentation fault.

When not creating a new shared_ptr, we don't set the ownerAddress, so it won't get deallocated or anything, that's fine, it works.

[HGuillemet]

Yes, but then when you pass that object to the function taking a shared_ptr as argument, a new shared_ptr will be created.
So the same pointer will be managed multiple times.
That's the point of PR #668 : to handle shared pointers correctly, we must never have a Java object without shared_ptr in ownerAddress.

[saudet]

Right, anyway please use SharedPtrAdapter to create one, not static_pointer_cast.

[HGuillemet]

We need both, don't we ? That's how it works currently in the PR.

[saudet]

I don't see why we need static_pointer_cast. We can do exactly the same thing with SharedPtrAdapter.

[saudet]

Or actually we'd need to extend SharedPtrAdapter and add a constructor with 2 types like the "possible implementation" here I guess: https://en.cppreference.com/w/cpp/memory/shared_ptr/pointer_cast#Version_1

[saudet]

Anyway, like I keep telling you, stop trying to do everything at the same time. You don't need to have all of it working exactly like it needs to be before we can merge. Let's get one thing at a time working.

[HGuillemet]

What bothers you with static_pointer_cast ? You're looking for something that would work for all adapters ?
That brings us back to my original question: how to detect if we need this adapter/sharedptr special cast or the normal static_cast ?

[HGuillemet]

Anyway, like I keep telling you, stop trying to do everything at the same time. You don't need to have all of it working exactly like it needs to be before we can merge. Let's get one thing at a time working.

The implementation of asX and the explicitCast feature seems related to me, but no problem for splitting that in two PR. Anything else you want me to separate ?

[saudet]

We should be able to use the adapter annotation from the constructor as is, that's what it's for. If you want to go into the details of all that and do a full POC before finalizing anything, that's fine, go ahead. I don't see where the problem is though. If you need to discuss this further, you'll need to provide more details about where you feel there is a problem.

[HGuillemet]

I don't see where the problem is though. If you need to discuss this further, you'll need to provide more details about where you feel there is a problem.

This PR currently generates either:

@Namespace public static native @Name("static_cast<B*>") B asB(D pointer);

or

@Namespace public static native @SharedPtr @Name("std::static_pointer_cast<B>") B asB(@Cast({"","std::shared_ptr<D>"}) @SharedPtr D pointer);

But, as you said, the second version could be generalized to something like:

@Namespace public static native @Adapter("AAA") @Name("AAA<B>::cast<D>") B asB(@Adapter("AAA") D pointer);

With the addition of a cast function in all adapters.
Did I understood well what you meant ?
It seems a good generalization to me. Would the cast do something meaningful for adapters others than sharedptr ?

Currently, the PR, to determine if the second version must be generated or the first one, looks for an annotation Info on the constructor of B or D. It looks clunky for me since:

• It should as well be B AND D.
• B and/or D could be abstract and the annotated constructor may be on subclasses (that's the case for Pytorch, where ReLUImpl inherits from Cloneable<ReLUImpl> which virtually inherits from Module. The annotated constructors are those of ReLUImpl and Module.)
• We look for an info on the argument-less cppname of the constructor, but this might be on the fullname info. We could even find constructors with the annotations, and some without.
• This is not like a annotation introspection, the test is based on String.contains. And we should handle @SharedPtr-style shortcuts somehow.

But if we cannot find something better and not too complicated, we can live with that I guess.

[saudet]

With the addition of a cast function in all adapters.
Did I understood well what you meant ?

Sort of, like I said, we should be able to add a constructor instead of a method.

This is not like a annotation introspection, the test is based on String.contains. And we should handle @SharedPtr-style shortcuts somehow.

We shouldn't need to introspect anything at all. Look, just try to make it work, if you're not able to make it work, we'll look at more concrete problems that you encounter when you encounter them. And like I keep telling you, why do you want to merge everything all at the same time? It makes it very hard to get anything done!

[HGuillemet]

I asked you above if you want me to split off something else of this PR than the asX implementation.

[saudet]

It's probably better that way yes, but if you want to have a complete POC that does absolutely everything you want, first, then by all means start working on that! As a first step, since I'm pretty sure we can do what you want to do with a new constructor to SharedPtrAdapter, please try to do that and see how that goes. Talking about every possible and impossible case isn't going to help.

[saudet]

Sorry, I think I'm starting to understand what you're saying. Basically, we need something like a function call for asXXX()... unless we start doing something special in Generator. But let's say we don't do that, how should the user be able to pass the name of that function call... Let me think about that

[HGuillemet]

For me this PR is the POC. It works with Pytorch and I'm currently using it. It's the last one of the series of PR (but probably not split enough) for consolidating Pytorch presets.

I realize I'm not very clear in my explanations. That could be interesting to generalize and pass an arbitrary function call for the cast. But:

• this information (how to cast an adapter of X to an adapter of Y) seems specific to adapters, and not presets-specific, so the solutions of an added constructor or static function to the adapters seems better.
• is it worth generalizing ? can you imagine a use case for other adapters than sharedptr ?

Concerning the split of this PR, I can cut it into

• explicitUpcast info and the wrapping stuff
• detection of virtual inheritance for issuing the warning/error
• asXX implementation
• removeAnnotations utility function
• support for @virtual

Is that ok ?

[saudet]

I like to consider one thing at a time, but if you're comfortable rewriting things, we can keep going in this thread, fixing one thing at a time, that's fine.

Let's see, about the static_pointer_cast function template, we can think of it as something that isn't specific to shared_ptr, it just currently doesn't apply to anything else than shared_ptr. So, what if we just use it whenever there is a user specified annotation on the constructor and that upcast mode is enabled? It wouldn't break anything, and we don't need to add anything about "shared_ptr" in the Parser and/or new knobs that are most likely never going to get used, for now anyway.

[HGuillemet]

I like to consider one thing at a time, but if you're comfortable rewriting things, we can keep going in this thread, fixing one thing at a time, that's fine.

Ok, then I'll just split off the detection of virtual inheritance for now. That's something optional that you can decide to merge or not, and that concerns separate lines of codes. The other features are more related to each others.

Let's see, about the static_pointer_cast function template, we can think of it as something that isn't specific to shared_ptr, it just currently doesn't apply to anything else than shared_ptr. So, what if we just use it whenever there is a user specified annotation on the constructor and that upcast mode is enabled? It wouldn't break anything, and we don't need to add anything about "shared_ptr" in the Parser and/or new knobs that are most likely never going to get used, for now anyway.

I agree that we can postpone generalization until the need arises, if ever.

Well, static_pointer_cast IS specific to shared pointers, whatever you think of it as, and we need the @SharedPtr annotation in this line of code. So you'll have something about shared_ptr in Parser anyway. Is this such a problem ? @SharedPtr annotation is part of core JavaCPP.
So since you'll have this bit of code specific to shared pointers, and you test for the presence of an annotation on the constructor, why not test if this annotation IS @SharedPtr ?

Let's sum up the implications of this static_pointer_cast:

1. it's needed when we want class B to be managed by shared pointer. The closest we have now to test this is the presence of a @SharedPtr annotation on B constructor.
2. It may be helpful when the instance of D contains a shared pointer, in case the resulting B is passed to a function taking a shared_ptr<B>. We cannot really test this situation. Can we ?
3. It's armful if the instance of D has an owner which is != address and is not a shared pointer. Since it'll be treated as a shared pointer by the adapter.

Can we assume that testing situation 1 is enough to exclude situation 3 ? I think so (although I'd prefer something to test on the class itself rather than on a constructor: an info or a @SharedPtr annotation).

We can check for explicitUpcast too to limit the risk, but this will exclude the following case:

• D multiple-inherit from B1 and B2.
• B2 is managed by shared pointers.

That could happen. If we limit to explicitUpcast, this case may trigger segmentation faults due to pointers owned by multiple shared pointers.

[HGuillemet]

I checked all presets. No change in parsing (besides @Virtual(true) now @Virtual(value = true)).

[saudet]

Well, static_pointer_cast IS specific to shared pointers, whatever you think of it as, and we need the @SharedPtr annotation in this line of code. So you'll have something about shared_ptr in Parser anyway. Is this such a problem ? @SharedPtr annotation is part of core JavaCPP. So since you'll have this bit of code specific to shared pointers, and you test for the presence of an annotation on the constructor, why not test if this annotation IS @SharedPtr ?

Many libraries use their own version of std::shared_ptr, such as OpenCV:
https://github.com/bytedeco/javacpp-presets/blob/master/opencv/src/main/resources/org/bytedeco/opencv/include/opencv_adapters.h

Let's sum up the implications of this static_pointer_cast:

1. it's needed when we want class B to be managed by shared pointer. The closest we have now to test this is the presence of a @SharedPtr annotation on B constructor.

No, it's only needed when we want to cast explicitly some shared_ptr. That only happens in Parser for multiple inheritance or that upcast mode, and yes we only want to do that when @SharedPtr is on the constructor.

2. It may be helpful when the instance of D contains a shared pointer, in case the resulting B is passed to a function taking a shared_ptr<B>. We cannot really test this situation. Can we ?

How is that not covered by your upcast mode?

3. It's armful if the instance of D has an owner which is != address and is not a shared pointer. Since it'll be treated as a shared pointer by the adapter.

Right, that's going to give a compiler error, so that's not a big issue.

Can we assume that testing situation 1 is enough to exclude situation 3 ? I think so (although I'd prefer something to test on the class itself rather than on a constructor: an info or a @SharedPtr annotation).

Ideally, sure, but whatever we come up with is unlikely to be used by anyone at all, so let's not fix things that aren't broken.

We can check for explicitUpcast too to limit the risk, but this will exclude the following case:

• D multiple-inherit from B1 and B2.

Sure, but that's alright for backward compatibility.

• B2 is managed by shared pointers.

static_pointer_cast doesn't get used unless there is multiple inheritance, so like I keep saying, you'll need to explain a bit more what you're worried about here.

That could happen. If we limit to explicitUpcast, this case may trigger segmentation faults due to pointers owned by multiple shared pointers.

Why? You mean we won't be changing the default behavior so that's a bad thing? There has been exactly zero issue reported about that, so there's no reason to fix what isn't broken.

[HGuillemet]

Many libraries use their own version of std::shared_ptr, such as OpenCV: https://github.com/bytedeco/javacpp-presets/blob/master/opencv/src/main/resources/org/bytedeco/opencv/include/opencv_adapters.h

Argh. I didn't realize that. I see cv::Ptr has its own staticCast, but that does a simple std::static_pointer_cast, which works bacause cv::Ptr inherits from std::static_pointer_cast.

So calling a constructor or static factory function on the adapter seems the best. We don't need to implement it on all adapters and let the compiler throws an error when it's needed and doesn't exist.
But we can also for now only support std::shared_ptr if you prefer.

Let's sum up the implications of this static_pointer_cast:

1. it's needed when we want class B to be managed by shared pointer. The closest we have now to test this is the presence of a @SharedPtr annotation on B constructor.

No, it's only needed when we want to cast explicitly some shared_ptr. That only happens in Parser for multiple inheritance or that upcast mode, and yes we only want to do that when @SharedPtr is on the constructor.

You need it in asX when you need all instances of class X to contain a shared_ptr. And you put @SharedPtr on the constructor for this reason. I think we agree.

2. It may be helpful when the instance of D contains a shared pointer, in case the resulting B is passed to a function taking a shared_ptr<B>. We cannot really test this situation. Can we ?

How is that not covered by your upcast mode?

if we generate the adapter-version of asX only when X constructor has the adapter, and if class B doesn't have it.
But this case can well never happen.
What I tried to explain is that ideally we should apply static_pointer_cast instead of static_cast when the argument of asX contains a shared_ptr. That can happen even if we didn't put the annotation on X constructor, or that the annotations is on a subclass constructor. But I don't see how we could make such test.

3. It's armful if the instance of D has an owner which is != address and is not a shared pointer. Since it'll be treated as a shared pointer by the adapter.

Right, that's going to give a compiler error, so that's not a big issue.

Won't it give a segmentation fault instead ? owner will be C-casted from void* to shared_ptr<D>*.

We can check for explicitUpcast too to limit the risk, but this will exclude the following case:

• D multiple-inherit from B1 and B2.

Sure, but that's alright for backward compatibility.

• B2 is managed by shared pointers.

static_pointer_cast doesn't get used unless there is multiple inheritance, so like I keep saying, you'll need to explain a bit more what you're worried about here.

That could happen. If we limit to explicitUpcast, this case may trigger segmentation faults due to pointers owned by multiple shared pointers.

Why? You mean we won't be changing the default behavior so that's a bad thing? There has been exactly zero issue reported about that, so there's no reason to fix what isn't broken.

It's not because there was no report that there is nothing to improve. You well see that if someone calls a asX on an instance containing a shared pointer, the shared pointer is lost. It's just that no one came into that situation, or into the one where it can trigger a segmentation fault. Or that the ones that did didn't bother to report and found a workaround, or just gave up.

The current behavior will be preserved in all cases since currently no constructor has annotations.

[saudet]

This looks pretty much good to merge. I've adjusted a few nits, so could you confirm that it still does what you need it to do, without changing the results for any of the presets?

[HGuillemet]

With last commit, still working and no changes in presets parsing compared to master.