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
lhs_constants
option for #1359
#1361
Conversation
@@ -2174,6 +2178,19 @@ def_optimize(AST_Binary, function(self, compressor) { | |||
self.left = make_node(AST_Undefined, self.left).optimize(compressor); | |||
if (self.operator.length == 2) self.operator += "="; | |||
} | |||
} else if (compressor.option("typeofs") |
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.
This block of code is only applicable for comparisons: true, typeofs: true, lhs_constants: false
. I'm not sure if the added complexity is worth it.
@@ -12,6 +12,7 @@ asm_mixed: { | |||
join_vars: true, | |||
keep_fargs: true, | |||
keep_fnames: false, | |||
lhs_constants: true, |
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.
When a test case already had several true
options, I didn't mind adding lhs_constants: true
.
@@ -182,11 +182,11 @@ static_means_execution: { | |||
let x = 0; | |||
// Does not get inlined as it contains an immediate side effect | |||
class WithStaticProps { | |||
static prop = (x = 0 === x ? 1 : "FAIL") |
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.
In this case, I opted to move the 0
back to the right-hand side. This test case is testing code unrelated to comparisons.
Nicely done, thanks! I'll release during the week |
I implemented a new option called
lhs_constants
which defaults totrue
. I thought about havingcomparisons: false
turn off left-hand side constants, but didn't like this as non-comparison operators (*
,&
,|
,^
) are also affected.That said, several
comparisons
optimizations assumelhs_constants: true
. I tried to decouple the two, but I could use some guidance: should we add more complexity incompress/index.js
to allowcomparisons: true, lhs_constants: false
? My assumption is that most people will either want bothtrue
or bothfalse
.I audited all failing test cases. I added
lhs_constants: true
for those which tested comparisons (or those that already had several other options set totrue
). When a test case focused on non-comparison optimizations, I reversed the order in the expected result.