Skip to content
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

Add object get property names #260

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
16 changes: 16 additions & 0 deletions context.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,22 @@ func valueResult(ctx *Context, rtn C.RtnValue) (*Value, error) {
return &Value{rtn.value, ctx}, nil
}

func valueStrings(ctx *Context, rtn C.RtnStrings) ([]string, error) {
if rtn.strings == nil {
return []string{}, newJSError(rtn.error)
}
length := rtn.length
slice := (*[1 << 28]*C.char)(unsafe.Pointer(rtn.strings))[:length:length]
var result []string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we know the size of result, we should be able to preallocate:

	result := make([]string, len(slice))
	for i := 0; i < len(slice); i++ {
		s := slice[i]
		defer C.free(unsafe.Pointer(s))
		result[i] = C.GoString(s)
	}
	defer C.free(unsafe.Pointer(rtn.strings))
	return result, nil

for i := 0; i < len(slice); i++ {
s := slice[i]
defer C.free(unsafe.Pointer(s))
result = append(result, C.GoString(s))
}
defer C.free(unsafe.Pointer(rtn.strings))
return result, nil
}

func objectResult(ctx *Context, rtn C.RtnValue) (*Object, error) {
if rtn.value == nil {
return nil, newJSError(rtn.error)
Expand Down
2 changes: 1 addition & 1 deletion deps/depot_tools
Submodule depot_tools updated from db41ee to 85d7fe
7 changes: 7 additions & 0 deletions object.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,13 @@ func (o *Object) SetIdx(idx uint32, val interface{}) error {
return nil
}

// Returns an array containing the names of the enumerable properties of this object,
// including properties from prototype objects
func (o *Object) GetEnumerablePropertyNames() ([]string, error) {
rtn := C.ObjectGetPropertyNames(o.ptr)
return valueStrings(o.ctx, rtn)
}

// SetInternalField sets the value of an internal field for an ObjectTemplate instance.
// Panics if the index isn't in the range set by (*ObjectTemplate).SetInternalFieldCount.
func (o *Object) SetInternalField(idx uint32, val interface{}) error {
Expand Down
22 changes: 22 additions & 0 deletions object_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package v8go_test

import (
"fmt"
"reflect"
"testing"

v8 "rogchap.com/v8go"
Expand Down Expand Up @@ -202,6 +203,27 @@ func TestObjectDelete(t *testing.T) {

}

func TestGetEnumerablePropertyNames(t *testing.T) {
t.Parallel()

ctx := v8.NewContext()
defer ctx.Isolate().Dispose()
defer ctx.Close()
val, _ := ctx.RunScript("const foo = {}; foo", "")
obj, _ := val.AsObject()
obj.Set("bar2", "baz2")
obj.Set("foo", "foobar")
obj.Set("hello", "world")

expectedProperties := []string{"bar2", "foo", "hello"}
properties, err := obj.GetEnumerablePropertyNames()
fatalIf(t, err)

if !reflect.DeepEqual(properties, expectedProperties) {
t.Error("properteis are not the same")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

properties*

}
}

func ExampleObject_global() {
iso := v8.NewIsolate()
defer iso.Dispose()
Expand Down
32 changes: 32 additions & 0 deletions v8go.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1297,6 +1297,38 @@ void ObjectSet(ValuePtr ptr, const char* key, ValuePtr prop_val) {
obj->Set(local_ctx, key_val, prop_val->ptr.Get(iso)).Check();
}

RtnStrings ObjectGetPropertyNames(ValuePtr ptr) {
LOCAL_OBJECT(ptr);
RtnStrings rtn = {};

Local<Array> names;
if (!obj->GetPropertyNames(local_ctx).ToLocal(&names)) {
rtn.error = ExceptionError(try_catch, iso, local_ctx);
return rtn;
}

uint32_t length = names->Length();
const char** strings = (const char**)malloc(length * sizeof(const char*));

for (uint32_t i = 0; i < length; i++) {
Local<Value> name_from_array;
if (!names->Get(local_ctx, i).ToLocal(&name_from_array)) {
for (i = i - 1; i > 0; i--) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this miss freeing strings[0] if the index doesn't get there? Should the middle param be i >=0

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

We can't use i >=0 though, since the integer is unsigned, so that would always be true. If we wanted to do that, we could change i to int64_t.

Would the code maybe be clearer if it matched the outer loop, e.g.

      length = i;
      for (i = 0; i < length; i++) {
        free(&strings[i]);

or by using a separate loop variable?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this inner loop is supposed to be from 0 to i whatever i is because it might need to free all strings if an error occurs which means that it might not be equal to length and we'd be trying to free elements we don't have yet.

Copy link
Collaborator

@genevieve genevieve Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it should be:

  for (uint32_t i = 0; i < length; i++) {
    Local<Value> name_from_array;

    if (!names->Get(local_ctx, i).ToLocal(&name_from_array)) {
      for (j = 0; j < i; j++) {
        free(&strings[j]);
      }
      free(strings);
      rtn.error = ExceptionError(try_catch, iso, local_ctx);
      return rtn;
    }
    String::Utf8Value ds(iso, name_from_array);
    strings[i] = CopyString(ds);
  }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this inner loop is supposed to be from 0 to i

right, which is my code snippet started with length = i;

Your suggestion is what I meant by "or by using a separate loop variable", except that the for loop line needs to declare the j variable by prefixing its assignment with its type: for (uint32_t j = 0; j < i; j++) {

Copy link
Collaborator

@genevieve genevieve Mar 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right let’s go with your suggestion. Think I got confused by the inner loop variable having the same name.

free(&strings[i]);
}
free(strings);
rtn.error = ExceptionError(try_catch, iso, local_ctx);
return rtn;
}
String::Utf8Value ds(iso, name_from_array);
strings[i] = CopyString(ds);
}

rtn.strings = strings;
rtn.length = length;
return rtn;
}

void ObjectSetIdx(ValuePtr ptr, uint32_t idx, ValuePtr prop_val) {
LOCAL_OBJECT(ptr);
obj->Set(local_ctx, idx, prop_val->ptr.Get(iso)).Check();
Expand Down
7 changes: 7 additions & 0 deletions v8go.h
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,12 @@ typedef struct {
RtnError error;
} RtnString;

typedef struct {
const char** strings;
int length;
RtnError error;
} RtnStrings;

typedef struct {
size_t total_heap_size;
size_t total_heap_size_executable;
Expand Down Expand Up @@ -271,6 +277,7 @@ int ValueIsModuleNamespaceObject(ValuePtr ptr);

extern void ObjectSet(ValuePtr ptr, const char* key, ValuePtr val_ptr);
extern void ObjectSetIdx(ValuePtr ptr, uint32_t idx, ValuePtr val_ptr);
RtnStrings ObjectGetPropertyNames(ValuePtr ptr);
extern int ObjectSetInternalField(ValuePtr ptr, int idx, ValuePtr val_ptr);
extern int ObjectInternalFieldCount(ValuePtr ptr);
extern RtnValue ObjectGet(ValuePtr ptr, const char* key);
Expand Down