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

[3.x] Fix parsing 'preload': increase/decrease parenthesis count #52521

Conversation

kdiduk
Copy link
Contributor

@kdiduk kdiduk commented Sep 9, 2021

This PR fixes #52499. The fix is very simple and safe - we just skip newline tokens if we encounter them after the '(' and before the ')'.
This fix is intended only for 3.4 version. The version 4.0 doesn't have this problem since all the parsing code is rewritten.

@kdiduk kdiduk requested a review from a team as a code owner September 9, 2021 20:51
@YuriSizov YuriSizov added this to the 3.4 milestone Sep 9, 2021
@akien-mga akien-mga changed the title #52499 Fix parsing 'preload': skip newlines after '(' and before ')' [3.x] Fix parsing 'preload': skip newlines after '(' and before ')' Sep 20, 2021
@vnen
Copy link
Member

vnen commented Sep 20, 2021

I feel like this is not the proper way to go in this case. If you have something like this:

preload(
    "res://"
    +
    "my_resource.tres"
)

It won't work with this fix since it only skip newlines right after the opening parenthesis and before the closing one. I know it's a weird construct but functions allow newlines anywhere in the argument expression, so preload should do the same.

A way to solve this would be to just increment and decrement the parenthesis variable like it's done when finding parentheses in the expression:

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.

Needs changes as advised by @vnen.

@kdiduk kdiduk force-pushed the 52499-preload-parsing-error-when-newline-encountered branch from 2477753 to 2dcb05a Compare October 5, 2021 21:12
@kdiduk
Copy link
Contributor Author

kdiduk commented Oct 5, 2021

I've updated the PR.
@vnen thank you very much for your help!

@kdiduk kdiduk force-pushed the 52499-preload-parsing-error-when-newline-encountered branch from 2dcb05a to 835143b Compare October 6, 2021 07:35
@akien-mga
Copy link
Member

Tested with a weird case, it seems to work well 👍

const enemy_class = preload(
	
	"res://" +
	
	# Hello :)
	
	"enemies/enemy.gd"
	# Hi!
	)

@akien-mga akien-mga merged commit 3f05c38 into godotengine:3.x Oct 6, 2021
@akien-mga
Copy link
Member

Thanks!

@kdiduk kdiduk changed the title [3.x] Fix parsing 'preload': skip newlines after '(' and before ')' [3.x] Fix parsing 'preload': increase/decrease parenthesis count Oct 6, 2021
@kdiduk kdiduk deleted the 52499-preload-parsing-error-when-newline-encountered branch October 6, 2021 12:21
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.

4 participants