-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
fix dirtybit #4095
fix dirtybit #4095
Conversation
I'll give this a try locally. |
Can confirm that this does indeed seem to fix #4094 |
@@ -248,7 +248,7 @@ export default class Renderer { | |||
.reduce((lhs, rhs) => x`${lhs} | ${rhs}`); | |||
} | |||
|
|||
return x`${dirty} & /*${names.join(', ')}*/ ${bitmask[0].n}` as BinaryExpression; | |||
return x`${dirty}[0] & /*${names.join(', ')}*/ ${bitmask[0].n}` as BinaryExpression; |
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.
If we're not doing the destructuring, do we even need a separate case for this now? (And do we need the concept of context_overflow
?)
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.
oh a good point, i didnt know what else context_overflow
is for, let me do a global search and understand them... 🤔
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.
If I remove the check for context_overflow
here and always act like there was an overflow, the tests still pass. It looks like the other place that context_overflow
is used is in conditionally constructing an array of -1
s to pass to the @init
helper. If nothing is passed, that argument defaults to [-1]
. I think that makes sense to continue to be a default (since a lot of the time there will be at most 31 things to track), but I think it would make sense to just do the <= 31 check there where we're (conditionally) filling the array, and get rid of context_overflow
.
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.
yup let me update them
Fix #4094
Found the problem is that we destructure the dirty bits array if it doesnt overflow.
but problem is, it is a number, instead of an object.
The
change
map can still be updated within thep
function. and previously is ok, because it is an object.Now, although the
dirty
bitmap array is updated, thedirty
is not updated, because we eagerly destructure it into a number.so after this change,
dirty
is an array, whether or not it is overflowed.