Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix loop-value-1-1 parsing #4282

Merged

Conversation

TPGamesNL
Copy link
Member

Description

The 'return null' here caused the parser to stop after the first match, even if the expression parsing failed.
For example, matching loop-value-1-1 against %number%-%number% has the first match of (loop-value)-(1-1), because that matches the pattern. However, if parsing the expression loop-value does not work, the parser is supposed to attempt (loop-value-1)-(1), because that also matches the pattern. Because of the return statement here, it doesn't do this.

The main problem with this change, is that it increases parsing time by about 17% from my tests.


Target Minecraft Versions: any
Requirements: none
Related Issues: #2382

@TPGamesNL TPGamesNL added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Aug 16, 2021
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

The increased parsing time is certainly not ideal, but perhaps unavoidable. Is this for every script that parsing time increases? Or ones that make use of specific syntax?

However, it is also worth nothing that there are other PRs open that will reduce parsing time as well.

That said, this gets approval anyways ;D

@TPGamesNL
Copy link
Member Author

The time increase would probably be for every script.

We should consider whether it's worth to fix this bug (there's always a workaround available using brackets) for a 17% increase in parsing time.

@TPGamesNL TPGamesNL merged commit ad7aff8 into SkriptLang:master Oct 30, 2021
@TPGamesNL TPGamesNL deleted the fix/loop-value-arithmetic-parsing branch October 30, 2021 10:37
TPGamesNL added a commit that referenced this pull request Oct 30, 2021
@moraedu moraedu mentioned this pull request Oct 6, 2023
1 task
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants