Skip to content
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

Merged
merged 3 commits into from Oct 17, 2020

Conversation

FLHerne
Copy link
Contributor

@FLHerne FLHerne commented Dec 7, 2019

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.

Copy link
Contributor

@planetmaker planetmaker left a 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):
Copy link
Contributor

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?

Copy link
Contributor Author

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.

LordAro
LordAro previously approved these changes Dec 28, 2019
@FLHerne
Copy link
Contributor Author

FLHerne commented Apr 26, 2020

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...

@Yexo
Copy link
Contributor

Yexo commented Apr 26, 2020

Nice cleanup, the new code is much easier to follow without all the boilerplate.

@FLHerne
Copy link
Contributor Author

FLHerne commented Apr 26, 2020

Note: I did consider implementing actual operators on Expression, but ultimately decided it would be confusing.

@FLHerne FLHerne changed the title Binop cleanup Reduce boilerplate for creating BinOps May 8, 2020
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.
Copy link
Contributor

@glx22 glx22 left a 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).

@FLHerne
Copy link
Contributor Author

FLHerne commented Oct 17, 2020

Thanks.

I'm confident the circular import isn't an actual problem -- nothing from binop is actually used in execution within nmlop or vice versa at import time. Moving the import inside the function would be a performance hit, and all my other ideas are intrusive enough to be another PR.

@FLHerne FLHerne merged commit 316106d into OpenTTD:master Oct 17, 2020
@FLHerne FLHerne deleted the binop-cleanup branch April 2, 2021 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants