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

Rename function parameters using a reserved keyword #757

Merged
merged 6 commits into from
Jun 7, 2024

Conversation

HGuillemet
Copy link
Contributor

Appends a _ to the name of function parameters when they clash with a Java reserved keyword.

@saudet
Copy link
Member

saudet commented May 24, 2024

@HGuillemet
Copy link
Contributor Author

Ok, I hadn't realized that the infoMap was queried for function parameter names.

@saudet
Copy link
Member

saudet commented May 26, 2024

It should, for that particular case, it's not?

@HGuillemet
Copy link
Contributor Author

Yes it is. New commit adds _ to parameter names like interface.

@HGuillemet
Copy link
Contributor Author

Added another small fix to prevent generation of successive ... in containers

@HGuillemet
Copy link
Contributor Author

Since this PR is still opened, adding another small fix:
When annotations are defined in info on constructors, they are set on allocate.
However, there is at least one annotation we want on the Java constructor: @Deprecated. This commits fixes that. Not in a very nice way. If you have an idea about how to do it better...

@saudet
Copy link
Member

saudet commented Jun 4, 2024

What happens if we don't use filterJavaAnnotations()? Let's name it maybe constructorAnnotations()?

@HGuillemet
Copy link
Contributor Author

Without the commit, if we have an info like new Info("deprecated").annotations("@Deprecated")) and a C++ constructor is deprecated, the @Deprecated annotation goes on allocate and not on the public Java constructor.
The idea of filterJavaAnnotations is to retain from a string of annotations only those that are meaningful for non-native Java methods, thus the name.
I only though of @Deprecated but there may be others. Ideally this would be all non-JavaCPP annotations.

`

@saudet
Copy link
Member

saudet commented Jun 4, 2024

Right, but can we just leave all annotations without any issues? Does it break something?

@HGuillemet
Copy link
Contributor Author

HGuillemet commented Jun 4, 2024

Ah. You mean place all annotations on both allocate and the constructor ?
There are already the JavaCPP parameter annotations that are placed uselessly on constructor parameters, so why not also place the method annotations, if you prefer...

@saudet
Copy link
Member

saudet commented Jun 4, 2024

Right, it's not exactly useless, so let's so that for the method annotations too if it works

@HGuillemet
Copy link
Contributor Author

Does it break something?

In fact it does, because some annotations that are valid on allocate (an ElementType.METHOD) are not on constructors (an ElementType.CONSTRUCTOR), like @Adapter.
So I guess we must filter the annotations to place on constructor according to some white list (I suggested: @Deprecated only).

@saudet
Copy link
Member

saudet commented Jun 5, 2024 via email

@HGuillemet
Copy link
Contributor Author

Ok, Can you merge if you agree with these changes ? I'd like to push an update or Pytorch presets that uses them.

@saudet saudet merged commit a506820 into bytedeco:master Jun 7, 2024
11 checks passed
@HGuillemet HGuillemet deleted the reserved_keywords branch June 8, 2024 08:38
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