-
Notifications
You must be signed in to change notification settings - Fork 22
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
BAAS-20006: Update goja to latest (March 2023) - Part 2 #80
Conversation
…ssertConstructor. Closes dop251#419. (cherry picked from commit 7adb499) (cherry picked from commit a39f431)
…efined. Closes dop251#423. (cherry picked from commit 1444e6b) (cherry picked from commit 2efcb12)
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.
LGTM! Some qs but no blockers
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.
I'll blindly approve it cause I don't want to block it.
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.
Thanks for breaking up to the commits. I don't have much to add for the PR, just a few optional cosmetic fixes since it came from the original goja code
switch (*ap).(type) { | ||
case loadStack: | ||
*ap = loadThisStack{} | ||
case initStack: |
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.
[opt] we can group the no-ops together in the switch statement
@@ -576,56 +729,84 @@ func (s *scope) finaliseVarAlloc(stackOffset int) (stashSize, stackSize int) { | |||
} | |||
code := scope.prg.code | |||
base := scope.base | |||
if argsInStash { | |||
if this { | |||
if derivedCtor { |
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.
[opt] can combined this with the above check
if this && derivedCtor{
...
}
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.
Both very legitimate changes! I'm only NOT going to do it to minimize conflicts in the future. Although our diversions are not clear yet (working on it ...) those are the only places where we can 100% make these type of changes. Much appreciate either way!
Conflicts
Implemented classes (including private elements).
call.ctx
tobuiltin_function.go
on line 198. It won't look like a change because I ported it over from before.f._call
to now take a context param too. This was necessary because we don't passcall
as an argument anymore but we pass the function call arguments directly.common_eval
signature base on the neweval
function signature.MemUsage
functions around, nothing particularly crazy.Fixed "delete 'a'.prop". Optimised a[<const_string>] to a.<const_string>
The “array with non-numeric keys" needed a change. There was an optimization introduced with dot notation, before we had two string props:
x["a"] = 3
x["c"] = 3
Both were the last item added on the stack before we check the memory the only difference between the two mem checks was the added "abc" and the added number.
After the change this is not true anymore and elements that can use dot notation are not added to the stack anymore. This simply means that "abc" is now the last item on the stack before we call the mem check again which would have resulted in additional mem increase of 3 (i.e.
len("abc")
). To "even things out" the last item added before the mem check are assigned with a number key value which will isolate again "abc" and the added number so the resulting mem increase stays the same.Fixed Export() value for arrow functions and classes. Added Runtime.A…
AssertFunction
andAssertConstructor
which was encapsulated inrunWrapped
. I just had to make sure we were still passing the correct context along.Refactored conversion to primitive. Always use String() method if d…
BAAS
We have an evergreen patch here that's looking promising. The failing tests seem unrelated.