Add of for downcasting when c-style cast cannot work

[HGuillemet]

The asB() method generated by the Parser allows to upcast an instance of a class D to an instance of one of its base class B in the cases where C-style pointer cast (as used for instance in pointer cast constructors B(Pointer) does not work.

This is useful for multiple inheritance and for virtual inheritance from a polymorphic class.

But we lack the equivalent downcast.

struct B1 {
  int i;
};
struct B2 {
  int j;
};
struct D: public B1, public B2 {
  int k;
}

In Java, D extends B1 and has a asB2() method.
But if we have an instance b2 of B2 that we know is a D, we have no way to cast it to an instance of D. new D(b2) will lead to a segmentation fault.

For polymorphic classes, here is an example from Pytorch: LinearImpl extends Module (through LinearImplCloneable). Say we get a instance of Module from one of the many functions of libtorch returning a Module, we know it's a LinearImpl, and we need to call its forward function. We cannot. new LinearImpl(module).forward(tensor) triggers a segmentation fault (see issue bytedeco/javacpp-presets#1393).

This PR adds a static of(B) method, along with the asB() method, to perform the downcast. This static method does a static_cast/static_pointer_cast or a dynamic_cast/dynamic_pointer_cast. The C++ rules that determine which casting flavor to use are as follows:

• When downcasting through virtual inheritance, static cast cannot be used.
• Dynamic cast can only be used if the source class is polymorphic (having at least 1 virtual member, declared or inherited).

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 that upcast info is still needed because we most certainly need this information before we parse the subclass that declares a virtual inheritance and we realize upcast is needed. However I added a warning when we parse the subclass if the situation is detected and the upcast 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 a Pointer. 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:

public D(B pointer) { super(of(B)); }

and make of private, but it's not really satisfactory since it just duplicates the object returned by of. Any idea ?

[saudet]

What's wrong with doing it in a way like pull #554 ?

[saudet]

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.

[HGuillemet]

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.

[saudet]

Among probably other things, I think we can get away with only dynamic_cast, and if we do that, how much does it simplify?

[HGuillemet]

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);
      |       ^~~~~~~~~~~~~~~~~~~~

[saudet]

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?

[HGuillemet]

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.

[saudet]

So let's remove that flag entirely until it's necessary somehow for someone somewhere?

[HGuillemet]

polymorphic ? Ok, as you wish.

[saudet]

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

[HGuillemet]

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 ?

[HGuillemet]

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 ?

[saudet]

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.

[HGuillemet]

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).

[saudet]

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.

[HGuillemet]

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.

[saudet]

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 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.

Right, ok, so let's say, if doesn't break anything with the presets, we can leave like that, sounds good?

[HGuillemet]

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.

[saudet]

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

[HGuillemet]

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 ?

[saudet]

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?

[HGuillemet]

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.

[saudet]

This looks good to merge! Anything else you'd like to change before that though?

[HGuillemet]

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.