godotengine
Expression.parse bug fixes and support for underscores in numbers
- Fix #88121, Fix #81895
- The parser allowed nonsense expressions without throwing an error, such as
a aor5 a - Modify the parsing process to disallow consecutive identifiers or constants.
- The parser allowed nonsense expressions without throwing an error, such as
- Resolve #73816, Resolve #97713, Resolve #84131
- Ternary operations (i.e. if else) are not supported by the Expression class. However, trying to use them anyway is undefined behavior and only "works" because of the previously mentioned parser bug.
- Modify the tokenization process such that "if" and "else" identifiers cause an error, which is valuable feedback to users.
- Resolve #84298
- Underscores in numbers would get tokenized incorrectly, causing undefined behavior. For example, the number "1_000" would get tokenized as [constant of value
1, identifier of name_000], which is a nonsensical expression (think5 a). - Modify the tokenization process such that underscores are ignored in a number. (i.e.
1000==1_0_00,0xAB==0xA_B).
- Underscores in numbers would get tokenized incorrectly, causing undefined behavior. For example, the number "1_000" would get tokenized as [constant of value
Bugsquad Edit:
- Fixes https://github.com/godotengine/godot/issues/97324
Your changes are split across 6 commits, you will need to squash them into 1.
I also noticed that this PR does not contain test cases for the linked issues, it would be great if you could add some based on the other tests from the file (there already seems to be a test case for underscore number literals, but it only checks the returned error and not the parse result so that would need to be extended).
Your changes are split across 6 commits, you will need to squash them into 1.
I also noticed that this PR does not contain test cases for the linked issues, it would be great if you could add some based on the other tests from the file (there already seems to be a test case for underscore number literals, but it only checks the returned error and not the parse result so that would need to be extended).
Ok thank you for the feedback! I will get to work on that next week.
Thank you so much for all of your patience! I am still learning how to use git and how to manage the PR policies and whatnot, but I think I figured it out now. Working on the unit tests now!
Your PR seems to fail some of the existing test cases as well. Some stuff in there needs to be fixed, e.g. scientific number notation, some other failing cases seem somewhat questionable to me (e.g. 123 456 is tested to result in 123 but I'm inclined to say an error would be more intuitive) but since the expression behaviour isn't really well documented some core dev should take a look and make a decision on that.
Your PR seems to fail some of the existing test cases as well. Some stuff in there needs to be fixed, e.g. scientific number notation, some other failing cases seem somewhat questionable to me (e.g.
123 456is tested to result in 123 but I'm inclined to say an error would be more intuitive) but since the expression behaviour isn't really well documented some core dev should take a look and make a decision on that.
I just went through and added/modified several unit tests to have more consistent behavior. That one you mention resulted in 123 before too. I just modified it so it expects an error, which is what my code produces.
It should be passing all tests now. Let me know if there's any other changes I should make!