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

Improve function detection to avoid accidental conversion #75002

Merged

Conversation

Maran23
Copy link
Contributor

@Maran23 Maran23 commented Mar 16, 2023

Fixes: #74650

This PR improves the function detection in the project converter by checking if the function really is standalone and not part of a method or similar.
E.g. when checking and converting a function like connect(, we do not want to detect and convert a function like reconnect(.

We achieve this by checking the previous character when the function name is detected in a line.
To recognize the function as standalone and not as part of a name, the following previous characters are checked:

  • A-Za-z0-9 -> econnect or 9connect - not a function call
  • _ -> _connect - not a function call
  • $-> $connect - not a function call
  • @-> @connect - not a function call

Or in other words:

  • connect() is a function call
  • connect() is a function call
  • object.connect() is a function call

@YuriSizov YuriSizov added this to the 4.1 milestone Mar 16, 2023
@Maran23 Maran23 force-pushed the project-converter-function-detection branch from 444e457 to b98e643 Compare March 18, 2023 16:17
@akien-mga
Copy link
Member

akien-mga commented Mar 18, 2023

Technically other preceding characters like [ or % or + might be valid. It's maybe better to check that the preceding character is not alphanumeric or _? Thankfully Godot 3 doesn't support extended ASCII/Unicode identifiers.

Otherwise, the approach in this PR is probably "good enough" for the use case, so we can also stick to this if preferred.

@Maran23
Copy link
Contributor Author

Maran23 commented Mar 18, 2023

Technically other preceding characters like [ or % or + might be valid. It's maybe better to check that the preceding character is not alphanumeric or _?

I thought about this as well. But wasn't sure if I may miss something.
If I understand correctly, checking for A-Za-z0-9 and _ will catch all possible characters here?

@akien-mga
Copy link
Member

If I understand correctly, checking for A-Za-z0-9 and _ will catch all possible characters here?

Yeah that seems correct:

return (p_char >= '0' && p_char <= '9') || (p_char >= 'a' && p_char <= 'z') || (p_char >= 'A' && p_char <= 'Z') || p_char == '_';

There could also be things like $connect to fetch a node with name "connect" but that wouldn't have ( at the end so it should be fine.

@Maran23 Maran23 force-pushed the project-converter-function-detection branch from b98e643 to 28e451d Compare March 18, 2023 19:46
@Maran23
Copy link
Contributor Author

Maran23 commented Mar 18, 2023

Yeah that seems correct:

return (p_char >= '0' && p_char <= '9') || (p_char >= 'a' && p_char <= 'z') || (p_char >= 'A' && p_char <= 'Z') || p_char == '_';

Changed the logic to match the if statement above. This is approach is much safer. Thanks!

@Maran23 Maran23 force-pushed the project-converter-function-detection branch from 28e451d to 4b32ad5 Compare March 18, 2023 19:50
@Maran23
Copy link
Contributor Author

Maran23 commented Mar 18, 2023

There could also be things like $connect to fetch a node with name "connect" but that wouldn't have ( at the end so it should be fine.

I think it makes sense to also check for the "$" sign. Since in this case it is also not a function call.

@Maran23 Maran23 force-pushed the project-converter-function-detection branch from 4b32ad5 to e7acb2a Compare March 18, 2023 20:28
@Maran23 Maran23 force-pushed the project-converter-function-detection branch from e7acb2a to 7829dff Compare April 1, 2023 13:32
@Maran23 Maran23 force-pushed the project-converter-function-detection branch from 7829dff to 2db1db9 Compare April 10, 2023 16:30
@Maran23 Maran23 force-pushed the project-converter-function-detection branch from 2db1db9 to d9ee20f Compare April 30, 2023 13:40
@Maran23 Maran23 force-pushed the project-converter-function-detection branch from d9ee20f to 76c2c7a Compare June 12, 2023 17:16
When converting a function like "connect(", we do not want to detect a function like "reconnect(" as a possible candidate for conversion.
@Maran23 Maran23 force-pushed the project-converter-function-detection branch from 76c2c7a to bd599d0 Compare June 12, 2023 17:19
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay reviewing. Looks good to me!

@akien-mga akien-mga merged commit fad039b into godotengine:master Jun 12, 2023
@akien-mga
Copy link
Member

Thanks!

@Reneator
Copy link

Thanks so much!

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

Successfully merging this pull request may close these issues.

Converter 3 -> 4: Replaces all connect() functions although not signal
4 participants