-
-
Notifications
You must be signed in to change notification settings - Fork 311
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
Fix preprocessor parenthesis expression parse #2236
Fix preprocessor parenthesis expression parse #2236
Conversation
…t/shaderlab-relative-include
…t/shaderlab-relative-include
WalkthroughThe Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev/1.3 #2236 +/- ##
===========================================
+ Coverage 68.27% 69.31% +1.04%
===========================================
Files 468 523 +55
Lines 24353 27309 +2956
Branches 3632 4069 +437
===========================================
+ Hits 16626 18929 +2303
- Misses 6400 6899 +499
- Partials 1327 1481 +154 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -382,7 +382,7 @@ export default class PpParser { | |||
if (scanner.getCurChar() === "(") { | |||
scanner.advance(); | |||
scanner.skipSpace(false); | |||
const ret = this._parseConstant(scanner); | |||
const ret = this._parseConstantExpression(scanner); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change from _parseConstant
to _parseConstantExpression
is appropriate for enhanced expression parsing.
The modification to use _parseConstantExpression
instead of _parseConstant
when parsing expressions within parenthesis is a logical enhancement. This change likely allows for more complex expressions to be correctly parsed, aligning with the intended improvements in the PR.
However, it is crucial to ensure that this change is thoroughly tested, especially since it affects the core functionality of the shader preprocessor.
Would you like me to help generate unit tests or update the documentation to reflect this change?
Please check if the PR fulfills these requirements
Summary by CodeRabbit