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
Reduce boilerplate for creating BinOps #70
Conversation
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.
Did you run checks on a few NewGRFs that results remain the same with an without patch?
I usually run OpenGFX+landscape (for snowline), OpenGFX+airports, FIRS and some vehicle NewGRF and a railtype NewGRF when I feel fancy so that I get all kind of coverage.
@@ -90,7 +90,7 @@ def reduce(self, id_dicts = [], unknown_id_fatal = True): | |||
# - If the operator allows it and the second expression is more complex than | |||
# the first one swap them. | |||
op = self.op | |||
if op in commutative_operators or self.op in (nmlop.CMP_LT, nmlop.CMP_GT): | |||
if op.commutative or op in (nmlop.CMP_LT, nmlop.CMP_GT): |
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.
Would it make sense to define a set (or another property like communative) called 'comparison' or similar?
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.
Just a "comparison" flag isn't really helpful, because you'd still need a table of which one to replace it with.
I looked at adding a swapped_operand_op
property to cover this, but it's awkward because Python doesn't have forward references...
There are several things in this file that could be done more nicely, but I don't think it's particularly in scope for this patch.
Rebased. I decided the changes I wanted to make were a mistake, so this is pretty much the same as before. I've tested with more grfs that this doesn't change the output. If possible I'd like to get this in before #103, because the rebase would be a nightmare. If that means I owe @LordAro to help rebase that instead, so be it, I think it would be easier... |
Nice cleanup, the new code is much easier to follow without all the boilerplate. |
Note: I did consider implementing actual operators on |
This allows, for example, `nmlop.AND(foo, 0x42)` instead of `BinOp(nmlop.AND, foo, ConstantNumeric(0x42, foo.pos), foo.pos)`
This should cause no changes in output.
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.
Adds a new CodeQL error, but this can be handled later.
Code is way easier to read (and write).
Thanks. I'm confident the circular import isn't an actual problem -- nothing from |
Should be no functional changes, except a few line positions might be tracked implicitly.
Last patch created mostly with some horrible sed expressions.
Regression tests pass.