Fix macro match bug when code token contains character . #2622
Fix macro match bug when code token contains character . #2622GuoLei1990 merged 1 commit intogalacean:mainfrom
. #2622Conversation
Sway007
added
bug
WalkthroughThe changes remove the period character ('.') from the set of characters recognized by the Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant LexerUtils
participant ShaderMacroProcessor
TestRunner->>LexerUtils: Tokenize shader source (macro.txt)
LexerUtils-->>TestRunner: Tokens (without recognizing '.')
TestRunner->>ShaderMacroProcessor: Process macros in tokens
ShaderMacroProcessor-->>TestRunner: Expanded shader code
TestRunner->>TestRunner: Compare output to expected result
Assessment against linked issues
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2622 +/- ##
==========================================
- Coverage 69.46% 69.03% -0.44%
==========================================
Files 961 961
Lines 100407 100406 -1
Branches 8697 8707 +10
==========================================
- Hits 69752 69312 -440
- Misses 30395 30834 +439
Partials 260 260
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/shader-lab/src/lexer/Utils.ts(0 hunks)tests/src/shader-lab/test-case/compare/macro.txt(1 hunks)tests/src/shader-lab/test-case/index.ts(1 hunks)tests/src/shader-lab/test-case/source/macro.txt(1 hunks)
💤 Files with no reviewable changes (1)
- packages/shader-lab/src/lexer/Utils.ts
🔇 Additional comments (2)
tests/src/shader-lab/test-case/index.ts (1)
7-7: Test case added for macro expansion with period character.This change correctly adds "macro.txt" to the test files array, extending test coverage to include verification of macro expansion containing the period character, which directly addresses the PR objective.
tests/src/shader-lab/test-case/compare/macro.txt (1)
1-4: Expected output after macro expansion looks correct.This file appropriately represents the expected output after macro expansion, where the
fragCoordmacro has been expanded inline. The period-containing accessor (.xy) is preserved in the expanded output, which is aligned with the PR's objective to fix macro expansion with period characters.
| #define fragCoord vec2(uv.x * iResolution.x, uv.y * iResolution.y) | ||
|
|
||
| vec2 q = fragCoord.xy / iResolution.xy; | ||
| vec2 p = ( 2.0 * fragCoord.xy - iResolution.xy ) / min( iResolution.y, iResolution.x ); No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Source test case valid for testing period character in macros.
The test case correctly defines a macro fragCoord on line 1 and uses it with property access (.xy) on lines 3-4, which tests the scenario described in the PR objective. This provides a good test case for verifying that macros containing period characters are properly expanded.
However, I notice that the actual implementation change to isPpCharacters (removing period from recognized characters) isn't included in the files for review. This is an important part of the fix.
🏁 Script executed:
#!/bin/bash
# Let's find and verify the changes to isPpCharacters function
rg -p "isPpCharacters" --type ts packages/Length of output: 261
Attention: Missing Implementation Update in isPpCharacters
The test file at tests/src/shader-lab/test-case/source/macro.txt successfully verifies macro expansion with period characters. However, after running the verification script, it appears that while the function isPpCharacters is present in both packages/shader-lab/src/preprocessor/PpScanner.ts and packages/shader-lab/src/lexer/Utils.ts, its implementation has not been updated to remove the period ('.') from the recognized characters. This part of the fix, which is critical for the PR objective, is currently missing.
-
Test Case:
- File:
tests/src/shader-lab/test-case/source/macro.txt - Status: Test case correctly validates macro expansion using a period.
- File:
-
Issue:
- File:
packages/shader-lab/src/lexer/Utils.ts - Concern: The implementation of
isPpCharactersdoes not reflect the change to exclude the period character.
- File:
Please update isPpCharacters to remove the period from the set of recognized characters as described in the PR objective.
GuoLei1990
changed the title
..
3 participants
Please check if the PR fulfills these requirements
WebGL doesn't support macros cotain character
.close #2620
Summary by CodeRabbit
New Features
Bug Fixes