-
Notifications
You must be signed in to change notification settings - Fork 157
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 support for assert through SymbolicCompare #2057
Conversation
@certik
I've chosen the first one as of now . I don't know of particular advantages to the second approach and the first approach was doable easily.
Once I implemented
But not
Well it does serve our purpose as of now which is using |
I think the new node SymbolicCompare is fine. We can later add SymbolicBinOp as well (right now we added it inside the IntrinsicFunction). Can you please separate this PR into two PRs, one for freeing variables, the other one for adding support for assert/compare? Keep the print statements in the test, they test that printing works. And the assert tests that the result what we print is correct. Otherwise it looks good! Yes, only support != and == for now. The other operators are meant to create an equation, not really a comparison that returns true or false. In fact, even != and == can be used to construct an equation, but I would keep the SymPy usage, where == and != return true or false, and is used to compare expressions. We'll use other mechanisms to construct an equation. |
Well most of the changes here are to support the assert statement . The code for freeing variables is actually quite minimal if you see.
Yes, this should be done ! I'll do it in the following commit |
It's hard to review multiple independent changes. Please separate the PRs, that way we can ensure that each change is clean and we can merge it. |
Sure I'll do that right now ! |
I've removed the changes related to freeing of variables , hope that makes it easier for you to review . Nothing changes, the tests should still pass as everything is same except we are not freeing the stack space at the end . I'll raise a pr for that as soon as this goes in. |
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.
I think this looks great!
@anutosh491 go ahead and submit a PR for the freeing of variables. |
I'll do it right away !Give me 5 minutes. |
The Pr adds support for the following
equal to
andnot equal to
operators throughassert
All test files have been updated to use assert