Code review fixes:

* Renamed jsonValue obj field to val
* Renamed jsonValue children field to owned + added an explicit comment about this field
* Got rid of unnecessary vm handle checks
* Renamed arg to param, identifiers to paramNames
* Raised maxID value to 100 000
* Removed freedIDs entry check

c-bindings/c-bindings.go

@@ -20,8 +20,9 @@ type vm struct {
 }
 
 type jsonValue struct {
-	obj      interface{}
+	val interface{}
-	children []*C.struct_JsonnetJsonValue
+	// these objects are exclusively owned by this jsonValue
+	owned []*C.struct_JsonnetJsonValue
 }
 
 var handles = handlesTable{}
@@ -166,21 +167,21 @@ func jsonnet_native_callback(vmRef *C.struct_JsonnetVm, name *C.char, cb *C.Json
 	p := unsafe.Pointer(params)
 	sz := unsafe.Sizeof(*params)
 
-	var identifiers ast.Identifiers
+	var paramNames ast.Identifiers
 
 	for i := 0; ; i++ {
-		arg := (**C.char)(unsafe.Pointer(uintptr(p) + uintptr(i)*sz))
+		param := (**C.char)(unsafe.Pointer(uintptr(p) + uintptr(i)*sz))
 
-		if *arg == nil {
+		if *param == nil {
 			break
 		}
 
-		identifiers = append(identifiers, ast.Identifier(C.GoString(*arg)))
+		paramNames = append(paramNames, ast.Identifier(C.GoString(*param)))
 	}
 
 	f := &jsonnet.NativeFunction{
 		Name:   C.GoString(name),
-		Params: identifiers,
+		Params: paramNames,
 		Func: func(x []interface{}) (interface{}, error) {
 			var (
 				arr     []*C.struct_JsonnetJsonValue
@@ -211,7 +212,7 @@ func jsonnet_native_callback(vmRef *C.struct_JsonnetVm, name *C.char, cb *C.Json
 				return nil, fmt.Errorf("failed to execute native callback, code: %d", success)
 			}
 
-			return v.obj, nil
+			return v.val, nil
 		},
 	}
 
@@ -220,9 +221,8 @@ func jsonnet_native_callback(vmRef *C.struct_JsonnetVm, name *C.char, cb *C.Json
 
 //export jsonnet_json_extract_string
 func jsonnet_json_extract_string(vmRef *C.struct_JsonnetVm, json *C.struct_JsonnetJsonValue) *C.char {
-	_ = getVM(vmRef) // Here we check it, is it ok?
 	v := getJSONValue(json)
-	str, ok := v.obj.(string)
+	str, ok := v.val.(string)
 
 	if !ok {
 		return nil
@@ -233,10 +233,9 @@ func jsonnet_json_extract_string(vmRef *C.struct_JsonnetVm, json *C.struct_Jsonn
 
 //export jsonnet_json_extract_number
 func jsonnet_json_extract_number(vmRef *C.struct_JsonnetVm, json *C.struct_JsonnetJsonValue, out *C.double) C.int {
-	_ = getVM(vmRef) // Here we check it, is it ok?
 	v := getJSONValue(json)
 
-	switch f := v.obj.(type) {
+	switch f := v.val.(type) {
 	case float64:
 		*out = C.double(f)
 		return 1
@@ -250,9 +249,8 @@ func jsonnet_json_extract_number(vmRef *C.struct_JsonnetVm, json *C.struct_Jsonn
 
 //export jsonnet_json_extract_bool
 func jsonnet_json_extract_bool(vmRef *C.struct_JsonnetVm, json *C.struct_JsonnetJsonValue) C.int {
-	_ = getVM(vmRef) // Here we check it, is it ok?
 	v := getJSONValue(json)
-	b, ok := v.obj.(bool)
+	b, ok := v.val.(bool)
 
 	if !ok {
 		return 2
@@ -267,10 +265,9 @@ func jsonnet_json_extract_bool(vmRef *C.struct_JsonnetVm, json *C.struct_Jsonnet
 
 //export jsonnet_json_extract_null
 func jsonnet_json_extract_null(vmRef *C.struct_JsonnetVm, json *C.struct_JsonnetJsonValue) C.int {
-	_ = getVM(vmRef) // Here we check it, is it ok?
 	v := getJSONValue(json)
 
-	if v.obj == nil {
+	if v.val == nil {
 		return 1
 	}
 
@@ -306,16 +303,15 @@ func jsonnet_json_make_array(vmRef *C.struct_JsonnetVm) *C.struct_JsonnetJsonVal
 func jsonnet_json_array_append(vmRef *C.struct_JsonnetVm, arr *C.struct_JsonnetJsonValue, v *C.struct_JsonnetJsonValue) {
 	_ = getVM(vmRef) // Here we check it, is it ok?
 	json := getJSONValue(arr)
-	slice, ok := json.obj.([]interface{})
+	slice, ok := json.val.([]interface{})
 
 	if !ok {
 		fmt.Fprintf(os.Stderr, "array should be provided")
 		os.Exit(1)
 	}
 
-	val := getJSONValue(v)
+	json.val = append(slice, getJSONValue(v).val)
-	json.obj = append(slice, val.obj)
+	json.owned = append(json.owned, v)
-	json.children = append(json.children, v)
 }
 
 //export jsonnet_json_make_object
@@ -332,24 +328,20 @@ func jsonnet_json_object_append(
 ) {
 	_ = getVM(vmRef) // Here we check it, is it ok?
 	d := getJSONValue(obj)
-	table, ok := d.obj.(map[string]interface{})
+	table, ok := d.val.(map[string]interface{})
 
 	if !ok {
 		fmt.Fprintf(os.Stderr, "object should be provided")
 		os.Exit(1)
 	}
 
-	val := getJSONValue(v)
+	table[C.GoString(f)] = getJSONValue(v).val
-	table[C.GoString(f)] = val.obj
+	d.owned = append(d.owned, v)
-	d.children = append(d.children, v)
 }
 
 //export jsonnet_json_destroy
 func jsonnet_json_destroy(vmRef *C.struct_JsonnetVm, v *C.struct_JsonnetJsonValue) {
-	_ = getVM(vmRef) // Here we check it, is it ok?
+	for _, child := range getJSONValue(v).owned {
-
-	for _, child := range getJSONValue(v).children {
-		// TODO what if a child is already freed?
 		jsonnet_json_destroy(vmRef, child)
 	}
 
@@ -361,9 +353,8 @@ func jsonnet_json_destroy(vmRef *C.struct_JsonnetVm, v *C.struct_JsonnetJsonValu
 	C.jsonnet_internal_free_json(v)
 }
 
-func createJSONValue(vmRef *C.struct_JsonnetVm, obj interface{}) *C.struct_JsonnetJsonValue {
+func createJSONValue(vmRef *C.struct_JsonnetVm, val interface{}) *C.struct_JsonnetJsonValue {
-	_ = getVM(vmRef) // Here we check it, is it ok?
+	id, err := handles.make(&jsonValue{val: val})
-	id, err := handles.make(&jsonValue{obj: obj})
 
 	if err != nil {
 		fmt.Fprintf(os.Stderr, err.Error())

c-bindings/handles.go

@@ -5,7 +5,7 @@ import (
 	"fmt"
 )
 
-const maxID = 1000
+const maxID = 100000
 
 // Because of Go GC, there are restrictions on keeping Go pointers in C.
 // We cannot just pass *jsonnet.VM/JsonValue to C. So instead we use "handle" structs in C
@@ -15,7 +15,7 @@ const maxID = 1000
 // become a problem.
 // The Handle IDs start with 1, so 0 is never a valid ID and the Handle's index in the array is (ID - 1).
 
-// handlesTable is the set of active, valid Jsonnet virtual machine handles allocated by jsonnet_make*.
+// handlesTable is the set of active, valid Jsonnet allocated handles
 type handlesTable struct {
 	objects  []interface{}
 	freedIDs []uint32
@@ -74,14 +74,5 @@ func (h *handlesTable) ensureValidID(id uint32) error {
 		return errInvalidHandle
 	}
 
-	// we should check freed ids too:
-	// * we should inform a client if there is an attempt to free already freed id
-	// O(len(freedIDs)) maybe need to use map?
-	for _, freed := range h.freedIDs {
-		if freed == id {
-			return errInvalidHandle
-		}
-	}
-
 	return nil
 }