Closed HGuillemet closed 1 year ago
What's wrong with doing it in a way like pull https://github.com/bytedeco/javacpp/pull/554 ?
I'm not entirely sure I understand what you're trying to do here, but it's obviously more complicated than it needs to be. I'll let you go through a few iterations of this until you settle on something that makes more sense.
I was probably not clear enough. What don't you understand in the purpose of this PR ?
How would you downcast a pytorch Module
to LinearImpl
in a simpler way ?
Basically, this PR just introduces the inverse of the asXXX
methods that perform an upcast in the case of multiple inheritance and virtual inheritance.
Among probably other things, I think we can get away with only dynamic_cast, and if we do that, how much does it simplify?
As said above, dynamic_cast won't work for non-polymorphic base class. So in the case of simple multiple inheritance, like in the example of the top post, downcasting a B2 to a D needs a static_cast:
Compiling this:
struct B1 {
int i;
};
struct B2 {
int j;
};
struct D: public B1, public B2 {
int k;
};
int main(int ac, char **av) {
D *d = new D();
B2 *b2 = d;
d = dynamic_cast<D*>(b2);
}
fails with:
error: cannot 'dynamic_cast' 'b2' (of type 'struct B2*') to type 'struct D*' (source type is not polymorphic)
15 | d = dynamic_cast<D*>(b2);
| ^~~~~~~~~~~~~~~~~~~~
I see, so there's too many types of casts to try and force this into never ending flags. How about we add an Info that would let users specify the name of the cast function (ehem, operator) they want to use for this?
The PR implements the needed logic to make the right choice between static_cast and dynamic_cast automatically, without needing info (but the new polymorphic info, added for good measure, but which may well be never used). It was tested in the different scenarios: polymorphic base or not, virtual inheritance or not.
Using an explicit info as you suggest would work, but why request that from the user when the parser can detect what to do by itself ? To save about ~ 10 new lines of code in parser ?
Also this new lines of code allows, as a side-effect, to emit a warning when upcast
info has not been provided when it should be.
So let's remove that flag entirely until it's necessary somehow for someone somewhere?
polymorphic ? Ok, as you wish.
But this behavior shouldn't be enabled by default. It only happens on Info.upcast right? That's a bit weird..
Anyway, from() sounds better than of() for that kind of operation: https://docs.oracle.com/javase/tutorial/datetime/overview/naming.html
But this behavior shouldn't be enabled by default. It only happens on Info.upcast right? That's a bit weird..
Which behavior ? The generation of the of/from methods ?
They are generated when asX methods are, so either with virtual inheritance (upcast
case), or multiple inheritance (automatic).
The method name upcast
in Parser should be renamed, as upAndDownCast
or whatever name you prefer.
Anyway, from() sounds better than of() for that kind of operation: https://docs.oracle.com/javase/tutorial/datetime/overview/naming.html
You must be right. However I think the best would be a constructor, as said in the last note or the top post. Any opinion or idea about that ?
But this behavior shouldn't be enabled by default. It only happens on Info.upcast right? That's a bit weird..
The upcast
name for the info turns out to be not a good choice either, with this new feature. polymorphicBaseClassWithDerivedClassesInheritingFromIt
probably not either. Any better idea ?
You must be right. However I think the best would be a constructor, as said in the last note or the top post. Any opinion or idea about that ?
I suppose you could add new constructors like you proposed? It creates a temporary wrapper object, but it can't hurt.
The
upcast
name for the info turns out to be not a good choice either, with this new feature.polymorphicBaseClassWithDerivedClassesInheritingFromIt
probably not either. Any better idea ?
Meh, Info.upcast is fine, let's not add anything more just for this. Ideally we'd want to always do all of that upcasting and downcasting I suppose, but for backward compatibility, let's just make sure upcast() doesn't get called without Info.upcast.
You must be right. However I think the best would be a constructor, as said in the last note or the top post. Any opinion or idea about that ?
I suppose you could add new constructors like you proposed? It creates a temporary wrapper object, but it can't hurt.
Ok. I guess we could add support for @Name
on allocate methods to directly do what is done by of/from in a constructor. Can you think of other uses that would make this addition to the generator worth ? Else let's stick with the wrapper, at least for now.
The
upcast
name for the info turns out to be not a good choice either, with this new feature.polymorphicBaseClassWithDerivedClassesInheritingFromIt
probably not either. Any better idea ?Meh, Info.upcast is fine, let's not add anything more just for this. Ideally we'd want to always do all of that upcasting and downcasting I suppose, but for backward compatibility, let's just make sure upcast() doesn't get called without Info.upcast.
The current upcast() is already called both for multiple inheritance and upcast
. It generates methods for upcasting only.
The PR adds the generation of reciprocal downcasting methods to it.
Do you mean that you prefer to generate the new downcast constructor for virtual inheritance/upcast
only and not for multiple inheritance ?
That seems weird for me, since that won't simplify the code. And the PR won't break anything, it fixes a "bug" triggered when calling the pointer cast constructor and causing a segmentation fault (even if nobody reported it yet).
Ok. I guess we could add support for
@Name
on allocate methods to directly do what is done by of/from in a constructor. Can you think of other uses that would make this addition to the generator worth ? Else let's stick with the wrapper, at least for now.
Sounds reasonable, @Name
isn't used for anything on allocate() methods right now. That could be used to call random create functions, to "objectify" C APIs and what not, as well.
The current upcast() is already called both for multiple inheritance and
upcast
. It generates methods for upcasting only. The PR adds the generation of reciprocal downcasting methods to it. Do you mean that you prefer to generate the new downcast constructor for virtual inheritance/upcast
only and not for multiple inheritance ? That seems weird for me, since that won't simplify the code. And the PR won't break anything, it fixes a "bug" triggered when calling the pointer cast constructor and causing a segmentation fault (even if nobody reported it yet).
No, I mean it shouldn't generate anything, unless the class has the Info.upcast flag explicitly set by the user.
Sounds reasonable,
@Name
isn't used for anything on allocate() methods right now. That could be used to call random create functions, to "objectify" C APIs and what not, as well.
Do you prefer I look into this and try to make a PR, or will you do it yourself ?
The current upcast() is already called both for multiple inheritance and
upcast
. It generates methods for upcasting only. The PR adds the generation of reciprocal downcasting methods to it. Do you mean that you prefer to generate the new downcast constructor for virtual inheritance/upcast
only and not for multiple inheritance ? That seems weird for me, since that won't simplify the code. And the PR won't break anything, it fixes a "bug" triggered when calling the pointer cast constructor and causing a segmentation fault (even if nobody reported it yet).No, I mean it shouldn't generate anything, unless the class has the Info.upcast flag explicitly set by the user.
I don't get it.
Classes with multiple inheritance already have the asB2()
method for upcasting to the class they don't inherit from in Java. This was the case before the addition of the upcast
info.
You propose to add the downcast constructor D(B2 b2)
only if we set Info.upcast on B2 ? This is weird.
Also I remind you that the upcast info does add the generation of asB()
upcast method, but it's main interest is to add wrappers to methods taking a B as argument so that we can pass a D without triggering a segmentation fault. This is for virtual inheritance only and doesn't concern multiple inheritance.
Do you prefer I look into this and try to make a PR, or will you do it yourself ?
It shouldn't be too hard, but it's not something I'll be doing myself, no...
I don't get it. Classes with multiple inheritance already have the
asB2()
method for upcasting to the class they don't inherit from in Java. This was the case before the addition of theupcast
info. You propose to add the downcast constructorD(B2 b2)
only if we set Info.upcast on B2 ? This is weird. Also I remind you that the upcast info does add the generation ofasB()
upcast method, but it's main interest is to add wrappers to methods taking a B as argument so that we can pass a D without triggering a segmentation fault. This is for virtual inheritance only and doesn't concern multiple inheritance.
Right, ok, so let's say, if doesn't break anything with the presets, we can leave like that, sounds good?
Right, ok, so let's say, if doesn't break anything with the presets, we can leave like that, sounds good?
Leave the PR as is ? Sure.
But let me have a look and see if I manage to add @Name
on allocate.
And we can rename method upcast (not info upcast), but that's a detail.
Leave the PR as is ? Sure. But let me have a look and see if I manage to add
@Name
on allocate. And we can rename method upcast (not info upcast), but that's a detail.
We probably only need to update this line: https://github.com/bytedeco/javacpp/blob/1.5.9/src/main/java/org/bytedeco/javacpp/tools/Generator.java#L2648
Right. That's what I did.
However, there is a little complication. Could you advise about it ?
Currently, when allocate
is annoted with an adapter, like @SharedPtr
, it constructs an adapter built with:
adapter(ptr, 1, NULL)
where ptr
results from a new
.
Here we want, in addition, to annotate with something like @Name("dynamic_pointer_cast<...>")
to replace the new
, so ptr
is a shared_ptr
.
We thus need to call the 1-argument constructor of the adapter.
What would be the best way to inform the generator about this ?
I'm not sure I follow, I didn't write that code. You're the one that added support for that, so do whatever it needs to do?
Ok.
The complication stems from the implementation of the support for adapters on constructors. It's not consistent with the way adapters works for normal functions.
With the new support for @Name
on constructor, in order to have the constructor create a shared pointer, it's more logical to write:
@SharedPtr @Name("std::make_shared<torch::nn::ReLUImpl") private native void allocate();
Rather than:
@SharedPtr private native void allocate();
I'll prepare a PR about that, but this will not be before the end of the week.
Nothing I can think of. We need to adapt the pytorch presets now, because the current version won't build correctly after this PR is merged.
The
asB()
method generated by the Parser allows to upcast an instance of a classD
to an instance of one of its base classB
in the cases where C-style pointer cast (as used for instance in pointer cast constructorsB(Pointer)
does not work.This is useful for multiple inheritance and for virtual inheritance from a polymorphic class.
But we lack the equivalent downcast.
In Java,
D
extendsB1
and has aasB2()
method. But if we have an instanceb2
ofB2
that we know is aD
, we have no way to cast it to an instance ofD
.new D(b2)
will lead to a segmentation fault.For polymorphic classes, here is an example from Pytorch:
LinearImpl
extendsModule
(throughLinearImplCloneable
). Say we get a instance ofModule
from one of the many functions of libtorch returning a Module, we know it's aLinearImpl
, and we need to call itsforward
function. We cannot.new LinearImpl(module).forward(tensor)
triggers a segmentation fault (see issue https://github.com/bytedeco/javacpp-presets/issues/1393).This PR adds a static
of(B)
method, along with theasB()
method, to perform the downcast. This static method does astatic_cast
/static_pointer_cast
or adynamic_cast
/dynamic_pointer_cast
. The C++ rules that determine which casting flavor to use are as follows:These rules imply that downcasting is not possible (without hacks we won't address) if inheritance is virtual but the base class is not polymorphic.
Virtual inheritance can be reliably detected by the parser. Polymorphism can be detected too, but not reliably since a class can inherit virtual members from a class defined in a non-parsed, or non-parsed-yet, header. This PR also adds these detections to determine if downcast can be performed and which flavor to use. It also adds a
polymorphic
info to label a class as polymorphic when the Parser cannot detect it. Not thatupcast
info is still needed because we most certainly need this information before we parse the subclass that declares a virtual inheritance and we realizeupcast
is needed. However I added a warning when we parse the subclass if the situation is detected and theupcast
info has not been set.Changes in existing presets: addition of
of
in presets with classes having multiple or virtual inheritance (hdf5, opencv, pytorch and tvm)COULD BE IMPROVED: It would be better if
of
were replaced by a constructor, acting as a specialization of the pointer cast constructor taking aPointer
. This way we limit the risk that the user triggers a segmentation fault by calling the pointer cast constructor when C-style cast won't work. But I don't see how to implement this easily as a constructor, without patching the generator. We could add something like:and make
of
private, but it's not really satisfactory since it just duplicates the object returned byof
. Any idea ?