-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
Implement set, get internal fields, & set internal field count #145
Conversation
Could I get a review on this as well, @dylanahsmith or @rogchap? |
object.go
Outdated
inserted := C.ObjectSetInternal(o.ptr, C.uint32_t(idx), value.ptr) | ||
|
||
if inserted == 0 { | ||
return errors.New("v8go: index exceeded internal field count") |
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.
These fields are internal to host/go code, so getting and setting them should be more like getting and setting a go slice. An index out of range error in Go is reported as a panic, so I think we should do the same here. This is because the error isn't something that we expect the caller to handle, but more an indication of a bug in the caller.
Likely the caller would be using these fields more like struct field anyways.
If the library user happens to be using these internal fields as a bounded array, then we could expose InternalFieldCount
so the assignment can be conditional (similar using the len
function on a slice)
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 pushed a commit to fix this, adding the panic and exposing the internal field count.
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.
Thank you, @dylanahsmith. I forgot to look into this.
Codecov Report
@@ Coverage Diff @@
## master #145 +/- ##
==========================================
+ Coverage 95.23% 95.38% +0.14%
==========================================
Files 12 12
Lines 483 498 +15
==========================================
+ Hits 460 475 +15
Misses 14 14
Partials 9 9
Continue to review full report at Codecov.
|
a6ffe11
to
d064e79
Compare
…ap#145) Co-authored-by: Dylan Thacker-Smith <[email protected]> (cherry picked from commit 19ec71c)
I've implemented the
Object.SetInternalField
,Object.GetInternalField
, and theObjectTemplate.SetInternalFieldCount
functions.Is this a viable solution or am I missing something?