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
Merged

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

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):

This comment has been minimized.

@planetmaker

planetmaker Dec 9, 2019
Contributor

Would it make sense to define a set (or another property like communative) called 'comparison' or similar?

This comment has been minimized.

@FLHerne

FLHerne Apr 26, 2020
Author Contributor

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 dismissed their stale review Jan 29, 2020

as requested

@FLHerne FLHerne force-pushed the FLHerne:binop-cleanup branch from 18ae289 to 3eb8dc8 Apr 26, 2020
@FLHerne
Copy link
Contributor Author

@FLHerne 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 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 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
@FLHerne FLHerne force-pushed the FLHerne:binop-cleanup branch from 3eb8dc8 to 3276541 May 18, 2020
FLHerne added 3 commits Dec 2, 2019
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.
@FLHerne FLHerne force-pushed the FLHerne:binop-cleanup branch from 3276541 to 2ac432c Oct 1, 2020
@glx22
glx22 approved these changes Oct 17, 2020
Copy link
Contributor

@glx22 glx22 left a comment

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 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
18 of 19 checks passed
18 of 19 checks passed
Commit checker
Details
Python 3.5 on ubuntu-latest
Details
Security and Quality
Details
Python 3.6 on ubuntu-latest
Details
Python 3.7 on ubuntu-latest
Details
Python 3.8 on ubuntu-latest
Details
Python pypy3 on ubuntu-latest
Details
Python 3.5 on macOS-latest
Details
Python 3.6 on macOS-latest
Details
Python 3.7 on macOS-latest
Details
Python 3.8 on macOS-latest
Details
Python 3.5 on windows-2016
Details
Python 3.6 on windows-2016
Details
Python 3.7 on windows-2016
Details
Python 3.8 on windows-2016
Details
Python 3.x on ubuntu-latest
Details
Python 3.x on macOS-latest
Details
Python 3.x on windows-2016
Details
CodeQL 1 error, 6 notes
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.