Added bitwise and or xor support #129 #126
Conversation
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Lexer
participant Parser
participant Expression
Lexer->>Parser: Tokenize input
Parser->>Expression: parse_expression()
Expression->>Parser: Call parse_binop()
Parser->>Expression: Handle binary operations
Expression->>Parser: Return binary operation nodes
Parser->>Lexer: Continue parsing
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- crosstl/src/backend/Opengl/OpenglLexer.py (2 hunks)
- crosstl/src/backend/Opengl/OpenglParser.py (4 hunks)
Additional comments not posted (6)
crosstl/src/backend/Opengl/OpenglLexer.py (2)
57-61: LGTM!The addition of new tokens for bitwise operations (
LEFTSHIFT,RIGHTSHIFT,BINOPAND,BINOPOR,BINOPXOR) is approved.
38-40: Verify the impact of removing assignment operators for bitwise operations.Ensure that the removal of
ASSIGN_BINOPOR,ASSIGN_BINOPAND, andASSIGN_BINOPXORdoes not break any existing functionality or cause any regressions.Run the following script to verify the usage of these assignment operators:
crosstl/src/backend/Opengl/OpenglParser.py (4)
362-364: LGTM!The code changes are approved. The addition of new assignment tokens expands the parser's capabilities and is implemented correctly.
560-567: LGTM!The code changes are approved. The new
parse_binopmethod is implemented correctly and enhances the parser's capabilities by handling binary operations.
610-610: LGTM!The code changes are approved. The modifications to the
parse_expressionmethod are consistent with the introduction of theparse_binopmethod and allow for more complex binary operations to be parsed directly.Also applies to: 623-624
Line range hint
1-1: Verify the impact of the changes and consider adding unit tests.The changes enhance the parser's capabilities for handling binary operations and are well-implemented. However, it's important to ensure that the modifications do not introduce any breaking changes or impact the existing functionality of the parser.
Consider adding unit tests to cover the new functionality and ensure that the existing tests still pass after the changes.
| @@ -35,6 +35,9 @@ | |||
| ("ASSIGN_SUB", r"-="), | |||
| ("ASSIGN_MUL", r"\*="), | |||
| ("ASSIGN_DIV", r"/="), | |||
| # ("ASSIGN_BINOPOR", r"\|="), | |||
There was a problem hiding this comment.
Hi,
Why are we commenting these out?
There was a problem hiding this comment.
HI Nripesh,
I need to implement them, i have only implemented binary ops not assignment binary ops. I'll do that by today.
| ("RIGHTSHIFT", r">>"), | ||
| ("BINOPAND", r"&"), | ||
| ("BINOPOR", r"\|"), | ||
| ("BINOPXOR", r"\^"), | ||
| ("MINUS", r"-"), |
There was a problem hiding this comment.
Hi Nripesh,
I have deleted those comments inside Lexer
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- crosstl/src/backend/Opengl/OpenglLexer.py (1 hunks)
Additional comments not posted (5)
crosstl/src/backend/Opengl/OpenglLexer.py (5)
54-54: LGTM!The code change is approved.
55-55: LGTM!The code change is approved.
56-56: LGTM!The code change is approved.
57-57: LGTM!The code change is approved.
58-58: LGTM!The code change is approved.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- crosstl/src/backend/Opengl/OpenglLexer.py (2 hunks)
- crosstl/src/backend/Opengl/OpenglParser.py (6 hunks)
Additional comments not posted (8)
crosstl/src/backend/Opengl/OpenglLexer.py (5)
38-40: LGTM!The code changes are approved.
57-57: LGTM!The code changes are approved.
58-58: LGTM!The code changes are approved.
59-61: LGTM!The code changes are approved.
62-62: The comments from previous reviews are no longer relevant as the user has addressed the issue.crosstl/src/backend/Opengl/OpenglParser.py (3)
326-369: LGTM!The changes to support bitwise XOR, OR, and AND assignment operations in the
parse_variablemethod look good.
591-598: LGTM!The new
parse_binopmethod implementation for handling bitwise AND, XOR, and OR operations looks good. It correctly parses the expressions and generates the AST while maintaining the precedence of operations.
Line range hint
641-655: LGTM!The changes in the
parse_expressionmethod to integrate the newparse_binopmethod look good. The method now correctly parses expressions containing bitwise operations.
ashwith2427
requested review from
samthakur587 and
vaatsalya123
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- crosstl/src/backend/Opengl/OpenglParser.py (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- crosstl/src/backend/Opengl/OpenglParser.py
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- crosstl/src/backend/Opengl/OpenglParser.py (5 hunks)
Additional comments not posted (1)
crosstl/src/backend/Opengl/OpenglParser.py (1)
592-599: LGTM!The new
parse_binopfunction and the corresponding changes in theparse_expressionmethod look good. The changes enhance the parser's functionality by allowing it to recognize and process a broader set of binary operation tokens (BINOPAND,BINOPXOR, andBINOPOR).Also applies to: 642-656
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- crosstl/src/backend/Opengl/OpenglParser.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- crosstl/src/backend/Opengl/OpenglParser.py
|
@ashwith2427 |
Yeah sure I will add them. |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- crosstl/src/backend/Opengl/OpenglParser.py (6 hunks)
- tests/test_backend/test_opengl/test_parser.py (1 hunks)
Additional comments not posted (3)
crosstl/src/backend/Opengl/OpenglParser.py (3)
326-369: LGTM!The code changes to handle new assignment tokens for bitwise operations in the
parse_variablemethod are implemented correctly and consistently.
594-601: LGTM!The new
parse_binopmethod correctly parses bitwise operations with the right precedence.
Line range hint
644-658: LGTM!The changes to the
parse_expressionmethod to callparse_binopinstead ofparse_additiveensure that bitwise operations are handled with the right precedence in expressions.
| def test_bitwise(): | ||
| code = """ | ||
| #version 450 | ||
| // Vertex shader | ||
| float perlinNoise(vec2 p) { | ||
| return fract(sin(dot(p, vec2(12.9898, 78.233))) * 43758.5453); | ||
| } | ||
| layout(location = 0) in vec3 position; | ||
| out vec2 vUV; | ||
|
|
||
| void main() { | ||
| vUV = position.xy * 10.0; | ||
| float noise = perlinNoise(vUV); | ||
|
|
||
| } | ||
| // Fragment shader | ||
| in vec2 vUV; | ||
| layout(location = 0) out vec4 fragColor; | ||
|
|
||
| void main() { | ||
| float noise = perlinNoise(vUV); | ||
| float height = noise * 10.0; | ||
| int y +=noise; | ||
| int k *= height; | ||
| int x |=y; | ||
| int j &=x; | ||
| int r |= (6|8*9)|8+6&0; | ||
| float s = (noise|(height&noise|height))*2|5*6; | ||
| vec3 color = vec3(height / 10.0, 1.0 - height / 10.0, 0.0); | ||
| } | ||
| """ | ||
|
|
||
| try: | ||
| tokens = tokenize_code(code) | ||
| parse_code(tokens) | ||
| except SyntaxError: | ||
| pytest.fail("Bitwise operations failedpytest") |
There was a problem hiding this comment.
Fix the syntax errors and typo in the test function.
The test function has the following issues:
- Line 273:
int y +=noise;is invalid. It should beint y = int(noise);ory += int(noise);ifyis already declared. - Line 274:
int k *= height;is invalid. It should beint k = int(height);ork *= int(height);ifkis already declared. - Line 278:
float s = (noise|(height&noise|height))*2|5*6;is invalid. Bitwise operations cannot be performed on floats. It should be changed to integer operations. - Line 287: The error message has a typo. It should be
"Bitwise operations failed"instead of"Bitwise operations failedpytest".
Apply this diff to fix the issues:
void main() {
float noise = perlinNoise(vUV);
float height = noise * 10.0;
- int y +=noise;
- int k *= height;
+ int y = int(noise);
+ int k = int(height);
int x |=y;
int j &=x;
int r |= (6|8*9)|8+6&0;
- float s = (noise|(height&noise|height))*2|5*6;
+ int s = (int(noise)|(int(height)&int(noise)|int(height)))*2|5*6;
vec3 color = vec3(height / 10.0, 1.0 - height / 10.0, 0.0);
}
"""
try:
tokens = tokenize_code(code)
parse_code(tokens)
except SyntaxError:
- pytest.fail("Bitwise operations failedpytest")
+ pytest.fail("Bitwise operations failed")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_bitwise(): | |
| code = """ | |
| #version 450 | |
| // Vertex shader | |
| float perlinNoise(vec2 p) { | |
| return fract(sin(dot(p, vec2(12.9898, 78.233))) * 43758.5453); | |
| } | |
| layout(location = 0) in vec3 position; | |
| out vec2 vUV; | |
| void main() { | |
| vUV = position.xy * 10.0; | |
| float noise = perlinNoise(vUV); | |
| } | |
| // Fragment shader | |
| in vec2 vUV; | |
| layout(location = 0) out vec4 fragColor; | |
| void main() { | |
| float noise = perlinNoise(vUV); | |
| float height = noise * 10.0; | |
| int y +=noise; | |
| int k *= height; | |
| int x |=y; | |
| int j &=x; | |
| int r |= (6|8*9)|8+6&0; | |
| float s = (noise|(height&noise|height))*2|5*6; | |
| vec3 color = vec3(height / 10.0, 1.0 - height / 10.0, 0.0); | |
| } | |
| """ | |
| try: | |
| tokens = tokenize_code(code) | |
| parse_code(tokens) | |
| except SyntaxError: | |
| pytest.fail("Bitwise operations failedpytest") | |
| def test_bitwise(): | |
| code = """ | |
| #version 450 | |
| // Vertex shader | |
| float perlinNoise(vec2 p) { | |
| return fract(sin(dot(p, vec2(12.9898, 78.233))) * 43758.5453); | |
| } | |
| layout(location = 0) in vec3 position; | |
| out vec2 vUV; | |
| void main() { | |
| vUV = position.xy * 10.0; | |
| float noise = perlinNoise(vUV); | |
| } | |
| // Fragment shader | |
| in vec2 vUV; | |
| layout(location = 0) out vec4 fragColor; | |
| void main() { | |
| float noise = perlinNoise(vUV); | |
| float height = noise * 10.0; | |
| int y = int(noise); | |
| int k = int(height); | |
| int x |=y; | |
| int j &=x; | |
| int r |= (6|8*9)|8+6&0; | |
| int s = (int(noise)|(int(height)&int(noise)|int(height)))*2|5*6; | |
| vec3 color = vec3(height / 10.0, 1.0 - height / 10.0, 0.0); | |
| } | |
| """ | |
| try: | |
| tokens = tokenize_code(code) | |
| parse_code(tokens) | |
| except SyntaxError: | |
| pytest.fail("Bitwise operations failed") |
There was a problem hiding this comment.
LGTM!!
@samthakur587
Please do a final review of the PR and merge it😁
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- crosstl/src/backend/Opengl/OpenglParser.py (6 hunks)
Additional comments not posted (4)
crosstl/src/backend/Opengl/OpenglParser.py (4)
326-328: LGTM!The addition of new assignment tokens and their corresponding binary operation nodes in the
parse_variablemethod looks good. The implementation follows the existing pattern and expands the parser's capabilities to handle bitwise assignment operations.Also applies to: 355-369
594-601: LGTM!The new
parse_binopmethod looks good. It introduces support for parsing bitwise operations and recursively builds the binary operation nodes, allowing for complex expressions. The implementation follows the pattern of other parsing methods.
644-644: LGTM!The changes in the
parse_expressionmethod to integrate the newparse_binopmethod look good. By callingparse_binopinstead ofparse_additive, bitwise operations are now included in the parsing of expressions. This change is consistent with the introduction of theparse_binopmethod and ensures proper handling of bitwise operations within expressions.Also applies to: 657-657
453-455: LGTM!The changes in the
parse_bodymethod to handle single-line and multi-line comments look good. By consuming theCOMMENT_SINGLEandCOMMENT_MULTItokens using theeatmethod, the parser effectively ignores the comments and continues parsing the rest of the function body. This improvement enhances the parser's ability to handle comments within function bodies.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/test_backend/test_opengl/test_parser.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/test_backend/test_opengl/test_parser.py
There was a problem hiding this comment.
hii @ashwith2427 changes is you made looking great to me i suggested few changes can you please add those changes. and also can you please change the codegen file also for this changes are generating the correct crossgl code . also we have different test files for the lexer , parser and codegen can you add test in all this file not just at parser file.
| @@ -35,6 +35,9 @@ | |||
| ("ASSIGN_SUB", r"-="), | |||
| ("ASSIGN_MUL", r"\*="), | |||
| ("ASSIGN_DIV", r"/="), | |||
| ("ASSIGN_BINOPOR", r"\|="), | |||
| ("ASSIGN_BINOPAND", r"&="), | |||
| ("ASSIGN_BINOPXOR", r"\^="), | |||
| ("EQUAL", r"=="), | |||
There was a problem hiding this comment.
hii @ashwith2427 can you please change the name of tokens to be like
("ASSIGN_AND", r"&="),
("ASSIGN_OR", r"\|="),
("ASSIGN_XOR", r"\^="),| ("RIGHTSHIFT", r">>"), | ||
| ("BINOPAND", r"&"), | ||
| ("BINOPOR", r"\|"), | ||
| ("BINOPXOR", r"\^"), |
There was a problem hiding this comment.
also for this Token name to be
("BITWISE_SHIFT_LEFT", r"<<"),
("BITWISE_SHIFT_RIGHT", r">>"),
("BITWISE_AND", r"&"),
("BITWISE_OR", r"\|"),
("BITWISE_XOR", r"\^"),| return AssignmentNode( | ||
| name, | ||
| BinaryOpNode(VariableNode(type_name, name), "BINOPAND", value), | ||
| ) | ||
| else: |
There was a problem hiding this comment.
can you please have one check and assign it as the current_token[0] in binop node operator node .
|
hii @ashwith2427 can you create a issue for each token you are adding support on which file lexer , parser , codegen so we can have track on this that this task is completed . i'll assign these issue to you once you created that issue . |
|
I have added the issue @samthakur587 |
ashwith2427
changed the title
There was a problem hiding this comment.
@samthakur587 I have fixed the code. I have also added the tests for both lexer and parser.
|
hii @ashwith2427 can you please edit your PR description and mention there the issue in which you are working on. |
|
@samthakur587 I have updated the PR Description |
|
|
||
| def uint_test(): | ||
| code = """ | ||
| #version 450 |
There was a problem hiding this comment.
why is this unit_test needed ? you only have to add the tests for you each token or issue you are working on like it is correct for bitwise but you also have to add the test like test_double , test_uint etc as a different function .
| @@ -301,5 +303,8 @@ def map_operator(self, op): | |||
| "NOT_EQUAL": "!=", | |||
| "AND": "&&", | |||
| "OR": "||", | |||
| "BITWISE_OR": "|", | |||
| "BITWISE_XOR": "^", | |||
| "BITWISE_AND": "&", | |||
| } | |||
There was a problem hiding this comment.
also add tests for codegen file also the changes you have mode.
| self.eat(op) | ||
| value = self.parse_expression() | ||
| if self.current_token[0] == "SEMICOLON": | ||
| self.eat("SEMICOLON") | ||
| return BinaryOpNode(VariableNode(type_name, name), op_name, value) | ||
| if op == "ASSIGN_ADD": |
There was a problem hiding this comment.
this will not work like that checking for each operator just use the assignment node and save the operator in the node. or use binaryopnode
| @@ -453,7 +514,7 @@ def parse_type(self): | |||
| elif self.current_token[0] == "IDENTIFIER": | |||
| type_name = self.current_token[1] | |||
| self.eat("IDENTIFIER") | |||
| if type_name in ["int", "float"]: | |||
| if type_name in ["int", "float", "unsigned int", "double"]: | |||
There was a problem hiding this comment.
unsigned int (opengl) -> uint (crossgl )
There was a problem hiding this comment.
yeah thats true iam mapping the types in codegen file. you can test it.
|
Hey @samthakur587 I have updated the code |
There was a problem hiding this comment.
hii @ashwith2427 great work till now we are very close to merge this PR. can you please made some changes that i have suggested .
| @@ -51,6 +56,11 @@ | |||
| ("OR", r"\|\|"), | |||
| ("NOT", r"!"), | |||
| ("PLUS", r"\+"), | |||
| ("BITWISE_SHIFT_LEFT", r"<<"), | |||
| ("BITWISE_SHIFT_RIGHT", r">>"), | |||
| ("BITWISE_AND", r"&"), | |||
There was a problem hiding this comment.
for these two token BITWISE_SHIFT_LEFT , BITWISE_SHIFT_RIGHT you havn't done anything as i have seen why have you added these in laxer token. either you can add support also for these two tokens in parser and codegen or you can just remove these token from here so that anyone else can work on this.
| try: | ||
| tokenize_code(code) | ||
| except SyntaxError: | ||
| pytest.fail("Bitwise operation failed") |
There was a problem hiding this comment.
also can you please add tests for unsigned int and double token also here
|
I have updated the code @samthakur587 |
for more information, see https://pre-commit.ci
| "ASSIGN_SUB": "-", | ||
| "ASSIGN_OR": "|", | ||
| "ASSIGN_XOR": "^", | ||
| "ASSIGN_AND": "&", |
There was a problem hiding this comment.
why here the bitwise_and and assign_and same and same thing for bitwise_or ,bitwise_and . the assign_and should be like &= and assign_or |= .
There was a problem hiding this comment.
Yeah in lexer it is &= and |= but when we map those we need to do & and then we assign right. Once can you check the result of assign.
Description
This PR adds the support for Bitwise AND,OR,XOR operations.It also adds assign operations for bitwise operators and support for unsigned integer, double data types.
This PR addresses and closes the following issues:
Bitwise AndToken at opengl backend #130Bitwise ORToken at opengl backend #131Bitwise XORToken at opengl backend #132Assignment AndToken at opengl backend #133Assignment ORToken at opengl backend #134Assignment XORToken at opengl backend #135