bytedeco / javacpp

The missing bridge between Java and native C++
Other
4.47k stars 582 forks source link

Copy declarations between classes #655

Open HGuillemet opened 1 year ago

HGuillemet commented 1 year ago

This PR adds a mechanism to copy declarations from one class to another. It is used automatically when a virtual inheritance towards a polymorphic class, that we cannot transpose to Java, is detected (see #651). We can also copy on-demand with a new Info.copyFrom. This is useful for instance when a C++ API forwards calls to another class. Example with Pytorch:

.put(new Info("torch::nn::Linear").copyFrom("torch::nn::LinarImpl::LinearImpl", "torch::nn::LinearImpl::forward"))
.put(new Info("torch::nn::Linear::forward").annotations("@Name(\"operator()\")"))        

copies the constructors and the forward method from LinearImpl to Linear, whatever their arguments are. We also map the new forward method to the operator() that does the C++ forwarding.

There is already the flatten info that replaces the inheritance of a superclass to all its subclasses by a copy of the declarations. It's not really the declarations that are copied, but the whole java text of the class, after a string replacement of the superclass name by the subclass name. The string replacement has good chance to break things like annotations, and there is no check for duplicate declarations after the copy. This PR keeps flatten() for compatibility but I'm not sure it's a good idea, because of its limits and because the code of Parser would be simpler if we merged this feature with the new copy mechanism. Is flatten() used (it is not in the current bytedeco presets) ? In which case ?

Limit: the headers must be parsed in order from superclasses to subclasses for the detection of polymorphic classes to work. Also the classes we copy from must be parsed before the classes we copy to.

Possible breakage: only in the case of virtual inheritance of a polymorphic class. The new style flattening will be triggered automatically.

Still work in progress because more tests are needed. Also I didn't bother yet with indentation of copied declaration or with copy of comments attached to declarations.

HGuillemet commented 1 year ago

654 was merged into this PR by mistake.

saudet commented 1 year ago

You're starting to change a lot of things here. Could you please make sure that the output of the current presets do not change?

HGuillemet commented 1 year ago

You're starting to change a lot of things here. Could you please make sure that the output of the current presets do not change?

I will, but maybe after you review the last changes, and comment about the points below... and if you don't prefer to reject the whole thing :smile:

The copy of declarations is used here for virtual inheritance and on-demand copy, but I believe it can be reused for other situations:

saudet commented 1 year ago

I will, but maybe after you review the last changes, and comment about the points below... and if you don't prefer to reject the whole thing smile

As long as it doesn't break anything, I think it's fine to add something like that, sure. But unless you're prepared to do a lot of testing to make sure it doesn't break someone's code somewhere, let's not break things intentionally like you're suggesting for flatten, etc.

HGuillemet commented 1 year ago

The needs behind this feature are partly covered by PR #671, now merged. Such feature may still be useful, but I think it's best to implement it when we rewrite the Parser.

So are you ok to close this PR ?