Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy declarations between classes #655

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

HGuillemet
Copy link
Contributor

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 HGuillemet changed the title Copy declaration between classes Copy declarations between classes Feb 18, 2023
@HGuillemet
Copy link
Contributor Author

#654 was merged into this PR by mistake.

@saudet
Copy link
Member

saudet commented Feb 19, 2023

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
Copy link
Contributor Author

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 😄

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

  • flattening. See comment in first message above. Shall we replace current flattening with copy of declarations ?
  • inheriting constructors using C++11 using. Currently the text of all constructors is saved in Info and copied to subclass declaring a using after a string replacement of class names, similarly to flattening. Besides possible failure of the string replacement, the problem is that we cannot copy, using the new mechanism, constructors that have been copied as text like this. For instance in Pytorch we won't be able to copy constructors from MaxPool3dImpl to MaxPool3d because MaxPool3dImpl declares a using MaxPoolImpl<3, MaxPool3dImpl>::MaxPoolImpl. We could store all constructor declarations in a map, to have them available for copy when a using is parsed, but then it's a big duplicate feature with the current storage of all constructor texts in Infos.
  • when a container type is added for smart pointers, for copying methods from the contained class to the container class ?

@saudet
Copy link
Member

saudet commented Feb 21, 2023

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 HGuillemet mentioned this pull request Apr 9, 2023
@HGuillemet
Copy link
Contributor Author

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants