-
Notifications
You must be signed in to change notification settings - Fork 570
Check for fields in externalize, internalize #824
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
Conversation
Check for fields before accessing first struct field in externalize and internalize functions. Fixes this bug: https://play.jsgo.io/308c189e795b573511cbc4d4606af6b0a60a065a Note: I don't have node so I can't generated the minified prelude.
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
CC @shurcooL
compiler/prelude/jsmapping.go
Outdated
@@ -113,6 +113,9 @@ var $externalize = function(v, t) { | |||
} | |||
return searchJsObject(v.$get(), t.elem); | |||
case $kindStruct: | |||
if (t.fields.length == 0) { |
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.
===
would be better
compiler/prelude/jsmapping.go
Outdated
@@ -347,6 +350,9 @@ var $internalize = function(v, t, recv) { | |||
case $kindPtr: | |||
return searchJsObject(t.elem); | |||
case $kindStruct: | |||
if (t.fields.length == 0) { |
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.
ditto
Oh I missed this comment. Then I'll try this later. If anyone can do this now, I'm also fine :-) |
Oooh I'll change |
Update prelude_min.go
ping @shurcooL |
Sorry about the delay. The code in There have been changes to how prelude is stored by now, so we'll need to rebase and regenerate it (as was mentioned in comments above) in order to make it mergeable. We should also add 2 tests for this bug/behavior. |
Ah right. Thank you. @dave I was wondering if you still can't do |
This should be ready to merge now I think? |
I think yes. |
Oh wait, @dmitshur suggested that we should add tests.
|
@dave We're looking at all old PRs, and see that this one is still outstanding. Unfortunately, the bug link in the original description has died. Are you able to either describe the bug this fixes, or provide a new reproduction case? Thanks. |
Superceded by #1194 |
Check for fields before accessing first struct field in externalize and internalize functions.
Fixes this bug: https://play.jsgo.io/308c189e795b573511cbc4d4606af6b0a60a065a
Note: I don't have node so I can't generated the minified prelude.