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

base_parser.py: Update precedence of == and != #684

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

ponchofigueroa
Copy link
Contributor

@ponchofigueroa ponchofigueroa commented Jan 3, 2025

To align with industry standard order of operations, and with EDKII parsers, equals (==) and not equals (!=) should have a higher precedence than logical OR (||) and logical AND (&&) operators.

Prior to this commit, they shared the same precedence level which would lead to evaluations similar to !if RACECAR == RACECAR || RACECAR == STREETCAR being completed in the wrong order as compared to EDKII parsers. Additionally, this removes the parentheses that were inserted at the beginning and end of tokens as they appear to serve no further purpose.

Copy link
Contributor

@Javagedes Javagedes left a comment

Choose a reason for hiding this comment

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

@ponchofigueroa to resolve the current CI check, simply run ruff format . at the workspace root of the repository (You may have to pip install ruff).

As for your changes, I've manually tested your test cases against the EDKII build parsers and the results are the same, so I appreciate you aligning us with them.

For brevity's sake, I would ask that you add a few more tests that use AND and OR instead of || and && and tests using != instead of ==.

We have had issues in the past of small changes in our conditional parser breaking scenarios with no tests, so I'd like to make sure we have tests for everything.

I have confidence in your changes not breaking the scenarios I'm asking for additional tests in - This is to ensure future changes do not break these scenarios 😄

@ponchofigueroa
Copy link
Contributor Author

Thanks @Javagedes, I've submitted a new version of the change that essentially recreates a truth table for each for the test I previously added. The new functions are a bit long for my liking, let me know if you think I went overboard and should simplify it somehow.

Do you have any comments on my removal of the parentheses at the start of _ConvertTokensToPostFix()? The change is unrelated but looked like a low hanging fruit. While inspecting the code I noticed the input list tokens is being modified, so when the function exits the list will have an extra ) at the end which is generally not a good practice. I believe the only reason to append ) is due to the stack being initialized with (, but I don't see the need to enclose the full expression in parentheses, so I removed them altogether.

Equals '==' operator should have a higher precedence than OR and AND.
This fixes a bug where a conditional like
"!if RACECAR == RACECAR ||  RACECAR == STREETCAR" would evaluate the last two
operators in the wrong order, returning False when it should be True.
Removing extra parenthesis at the end of the tokens as it is modifying the
input object and is only needed because an opening one is added to the stack.
Copy link

codecov bot commented Jan 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.44%. Comparing base (ce85203) to head (b828b1f).
Report is 125 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #684   +/-   ##
=======================================
  Coverage   81.43%   81.44%           
=======================================
  Files          56       45   -11     
  Lines        7514     7491   -23     
=======================================
- Hits         6119     6101   -18     
+ Misses       1395     1390    -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Javagedes
Copy link
Contributor

Thanks @Javagedes, I've submitted a new version of the change that essentially recreates a truth table for each for the test I previously added. The new functions are a bit long for my liking, let me know if you think I went overboard and should simplify it somehow.

Do you have any comments on my removal of the parentheses at the start of _ConvertTokensToPostFix()? The change is unrelated but looked like a low hanging fruit. While inspecting the code I noticed the input list tokens is being modified, so when the function exits the list will have an extra ) at the end which is generally not a good practice. I believe the only reason to append ) is due to the stack being initialized with (, but I don't see the need to enclose the full expression in parentheses, so I removed them altogether.

I also don't see any need for the ( and ) anymore. My guess is that it was a remnant of a previous implementation that is no longer needed as improvements were made to the parser. I'm fine with removing it. All of our tests pass. I'll keep an eye out for anyone reporting issues and can always revert that portion of the change.

Thanks for adding the extra tests, also.

@Javagedes Javagedes added the bug Something isn't working label Jan 3, 2025
@Javagedes Javagedes added this to the v0.22.4 milestone Jan 3, 2025
@Javagedes Javagedes changed the title Update operator precedence for post fix evaluation base_parser.py: Update precedence of logical OR and AND Jan 3, 2025
@Javagedes Javagedes changed the title base_parser.py: Update precedence of logical OR and AND base_parser.py: Update precedence of == and != Jan 3, 2025
@Javagedes Javagedes merged commit c4a8dd5 into tianocore:master Jan 3, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants