Skip to content

Incorrect renaming when the method/field already has a numeric suffix #571

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

Closed
Tracked by #1171
HosseinYousefi opened this issue Aug 18, 2023 · 5 comments · Fixed by #1530
Closed
Tracked by #1171

Incorrect renaming when the method/field already has a numeric suffix #571

HosseinYousefi opened this issue Aug 18, 2023 · 5 comments · Fixed by #1530
Assignees
Labels
package:jni package:jnigen type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@HosseinYousefi
Copy link
Member

For example, if we have three methods: overloaded, overloaded and overloaded1. The second overloaded gets converted to overloaded1 but overloaded1 stays the same.

@HosseinYousefi HosseinYousefi added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) package:jnigen labels Aug 18, 2023
@HosseinYousefi HosseinYousefi transferred this issue from dart-archive/jnigen Nov 17, 2023
@HosseinYousefi HosseinYousefi moved this to Backlog in JNIgen tracker Apr 11, 2024
@HosseinYousefi HosseinYousefi added this to the JNI / JNIgen 0.14.0 milestone May 22, 2024
@HosseinYousefi HosseinYousefi self-assigned this May 22, 2024
@HosseinYousefi HosseinYousefi moved this from Backlog to Todo in JNIgen tracker May 22, 2024
@HosseinYousefi HosseinYousefi moved this from Todo to In Progress in JNIgen tracker Jul 9, 2024
@HosseinYousefi
Copy link
Member Author

Multiple ways to solve this:

  • First come first serve: foo, foo, foo1 -> foo, foo1, foo2
    • Pro: Simplest.
    • Con: User would expect foo1 to be foo1 but it's not.
  • First all the unique names, then all the non-unique ones: foo, foo, foo1 -> foo, foo2, foo1
    • Pro: Less confusing foo1 remains foo1.
    • Con: Users would expect all these to be overloadings of foo, but the only overloading is foo2.
  • Use a separator: foo, foo ,foo1 -> foo, foo$1, foo1.
    • Pro: Most clear. Anything with $ is overloaded.
    • Con: Adds a symbol to all overloaded methods. Overloaded methods are used a lot, but methods that end with a number are rare.

@liamappelbe @dcharkes wdyt? What happens in ffigen? Maybe we should use the same technique.

@dcharkes
Copy link
Collaborator

dcharkes commented Sep 5, 2024

The second option would require multiple traversals with visitors and saving such information somewhere. Could be done, but complicates things for users.

A separator is not a bad idea, it also signals the rename to the user.

@HosseinYousefi
Copy link
Member Author

A separator is not a bad idea, it also signals the rename to the user.

Another way: methods with numeric suffices get an extra $ suffix.

foo, foo, foo1, foo1 -> foo, foo1, foo1$, foo1$1

As methods ending with a number are rare in Java (as Java has operator overloading), it's both correct and not ugly in the happy path.

@liamappelbe
Copy link
Contributor

In this case, ffigen's UniqueNamer would do foo, foo, foo1, foo1 -> foo, foo1, foo11, foo12 😆. It doesn't try to parse the numbers at the end of the name, it just adds an incrementing number to the end of whatever you give it, until it finds something not in its set.

@HosseinYousefi
Copy link
Member Author

HosseinYousefi commented Sep 6, 2024

I think I like the last way the best. @liamappelbe Feel free to do the same for ffigen to be consistent.

Although eventually I want to give the users the ability to change this through transformations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:jni package:jnigen type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants