-
Notifications
You must be signed in to change notification settings - Fork 61
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
Added else if Conditional Statements to the Metal backend #34 #39
Conversation
Can u please check what else is there to change in this code? |
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.
thanks @adityanarayanan343 for contributing here i'll happy to solve any query if you in understanding the code.
if self.code[pos : pos + 7] == "else if": | ||
self.tokens.append(("ELSE_IF", "else if")) | ||
pos += 7 | ||
continue | ||
for token_type, pattern in TOKENS: |
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.
hii @adityanarayanan343 here you have added a condition for check else if
token. you can directly add Token in TOKENS list and in this MetalLexer class the function tokenize will compile the string and find the if this token is in the code.
f"IfNode(condition={self.condition}, if_body={self.if_body}, " | ||
f"elif_conditions={self.elif_conditions}, " | ||
f"elif_bodies={self.elif_bodies}, else_body={self.else_body})" | ||
) | ||
|
||
|
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.
the changes in IfNode class is lgtm in ast.
Can you check whether all the fixes you mentioned were addressed and please do tell if there were any other issues. |
Hii @adityanarayanan343 sorry for late reply your changes are looking great 👍 One last thing can you please add a test for your else_if condition at metal backend you can find test files in tests folder. Before that pull the latest changes. 😄 Thanks |
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.
LGTM 🚀 waiting for tests to be added
Sure thing @samthakur587 I will do it tomorrow morning and create a pull request. |
You can write all tests in this pr also just pull latest changes add test and push on this branch. |
Hi @samthakur587 I have done testing and added if- |
hii @adityanarayanan343 the tests are passing at metal codegen can you please have a look on this self = <crosstl.src.backend.Metal.MetalCrossGLCodeGen.MetalToCrossGLConverter object at 0x7f10543909a0>
E AttributeError: 'IfNode' object has no attribute 'body' crosstl/src/backend/Metal/MetalCrossGLCodeGen.py:178: AttributeError |
Hi @samthakur587 , I think I have fixed the issue so can you please confirm the same thing and do comment on it. Kudos, |
hii @adityanarayanan343 your tests are passing but not translating the correctly
in else if condition the only |
Hi @samthakur587 I have added a separate case for if and if-else conditions please check this issue and check for any other issues. Also, can you show me where the else if issue [ ] is showing because I can't find it Aditya |
Hi @vaatsalya123 can you check whether this issue has been fixed or not and can you please comment on it. Thanks, |
hii @adityanarayanan343 if you run this command . pytest -v tests/test_backend/test_metal/test_codegen.py -s then you can see the generated code for your test and if you see for else_if test then in this the condition is generated empty . |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
hii @adityanarayanan343 we are very close to merging the PR . just make the last changes i requested .
if_body = self.parse_block() | ||
|
||
# Handle `else if` | ||
elif_conditions = [] |
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.
hii @adityanarayanan343 can you have check on this bcz you are combining the elif_body and elif_condition in single list elif_condition which is not a good way to storing it in ast node . even when you have created the 2 different parameters in IFNode is elif_condition
and elif_body
.
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.
Hi @samthakur587, I think I have added the required changes please do check on it and comment on it.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
hii @adityanarayanan343 great work i have fixed some small issues . now everything LGTM 🚀
Added Support for else if in MetalParser
Grammar Update: Enhanced the parse_if_statement method to handle else if constructs in Metal syntax.
AST Update: Modified the Abstract Syntax Tree (AST) structure to support else if nodes, ensuring correct hierarchical representation of nested conditional logic.
Code Refactor: Refactored the parse_if_statement method to improve readability and maintainability while integrating else if logic.
Closes #34