Closed HGuillemet closed 1 year ago
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 off
), in order to still support virtualization, we add an optional parameterjavaName
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 ofasX
that can takes care of smart pointers (could be in a separate PR), by using adapters and replacing thestatic_cast
by astatic_pointer_cast
. However this still need improvements because:
it only supports
shared_ptr
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!
- Since the native method doesn't have its original name (
_f
instead off
), in order to still support virtualization, we add an optional parameterjavaName
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 ofasX
that can takes care of smart pointers (could be in a separate PR), by using adapters and replacing thestatic_cast
by astatic_pointer_cast
. However this still need improvements because:
- it only supports
shared_ptr
- 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.
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.
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.
Right, anyway please use SharedPtrAdapter to create one, not static_pointer_cast.
We need both, don't we ? That's how it works currently in the PR.
I don't see why we need static_pointer_cast. We can do exactly the same thing with SharedPtrAdapter.
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
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.
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 ?
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 ?
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.
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:
ReLUImpl
inherits from Cloneable<ReLUImpl>
which virtually inherits from Module
. The annotated constructors are those of ReLUImpl
and Module
.)But if we cannot find something better and not too complicated, we can live with that I guess.
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!
I asked you above if you want me to split off something else of this PR than the asX implementation.
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.
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
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:
Concerning the split of this PR, I can cut it into
explicitUpcast
info and the wrapping stuffIs that ok ?
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.
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
:
@SharedPtr
annotation on B constructor. shared_ptr<B>
. We cannot really test this situation. Can we ?!= 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:
That could happen. If we limit to explicitUpcast
, this case may trigger segmentation faults due to pointers owned by multiple shared pointers.
I checked all presets. No change in parsing (besides @Virtual(true)
now @Virtual(value = true)
).
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 aboutshared_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
:
- 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.
- 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?
- 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.
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
:
- 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.
- 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.
- 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.
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?
With last commit, still working and no changes in presets parsing compared to master.
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:
explicitUpcast
is activated, so there is little chance of breaking anythingHow it works:
explicitUpcast
is used to mark those classes that need astatic_cast
for converting a pointer of a subclass to a pointer of this class.asB()
methods are added for all marked classesB
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 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); }