Skip to content
This repository was archived by the owner on Mar 8, 2020. It is now read-only.

uast: properly load node arrays with registered types #344

Merged
dennwc merged 1 commit into from
Jan 16, 2019
Merged

uast: properly load node arrays with registered types #344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions uast/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,10 +237,11 @@ func fieldName(f reflect.StructField) (string, bool, error) {
}

var (
reflString = reflect.TypeOf("")
reflAny = reflect.TypeOf((*Any)(nil)).Elem()
reflNode = reflect.TypeOf((*nodes.Node)(nil)).Elem()
reflNodeExt = reflect.TypeOf((*nodes.External)(nil)).Elem()
reflString = reflect.TypeOf("")
reflAny = reflect.TypeOf((*Any)(nil)).Elem()
reflAnySlice = reflect.TypeOf([]Any{})
reflNode = reflect.TypeOf((*nodes.Node)(nil)).Elem()
reflNodeExt = reflect.TypeOf((*nodes.External)(nil)).Elem()
)

// ToNode converts generic values returned by schema-less encodings such as JSON to Node objects.
Expand Down Expand Up @@ -384,6 +385,26 @@ func setAnyOrNode(dst reflect.Value, n nodes.External) (bool, error) {
}
dst.Set(reflect.ValueOf(nd))
return true, nil
} else if rt == reflAnySlice {
narr, ok := n.(nodes.ExternalArray)
if !ok {
return false, nil
}
sz := narr.Size()
arr := make([]Any, 0, sz)
Copy link
Contributor

@creachadair creachadair Jan 14, 2019

Choose a reason for hiding this comment

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

Since you know the exact size anyway, and there's no early exit, you might as well just allocate and assign rather than appending: make([]Any, sz) … arr[i] = v

Copy link
Member Author

@dennwc dennwc Jan 15, 2019

Choose a reason for hiding this comment

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

I usually prefer not to. Because an exact size is only an optimization, and shouldn't affect the code below.

Copy link
Contributor

@creachadair creachadair Jan 15, 2019

Choose a reason for hiding this comment

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

Generally if you don't care about optimality, I recommend the simpler code: var arr []Any. The append builtin reallocates intelligently to avoid quadratic moves. If the allocation really matters, it's better not to append at all, and assign directly. The only time this pattern really makes sense is if you're copying out of a map, where there is no index without a separate explicit induction variable.

Copy link
Member Author

@dennwc dennwc Jan 15, 2019

Choose a reason for hiding this comment

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

It's more like a middle ground. When I don't care about optimality, I use var arr []Any, but when I do care, I simply change that single line to arr := make([]Any, 0, sz) and don't touch the code below (it's valid for both cases).

You may argue that it's might be less optimal than a direct assignment, since append will increment the counter, but it should be negligible comparing to any allocation and the code is a bit more flexible in case some decide to add additional items. I think it's a good balance, but maybe I should use the optimal one, as you suggest.

Copy link
Contributor

@creachadair creachadair Jan 15, 2019

Choose a reason for hiding this comment

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

I guess my point is that there's no real need to add an explicit allocation at all, if the cost is negligible (which, in this case, it likely is). The explicit allocation is a red flag to the reader that something interesting is happening—when in fact it's not really. :)

for i := 0; i < sz; i++ {
e := narr.ValueAt(i)
var v Any = e
if nv, err := NewValue(TypeOf(e)); err == nil {
if err = nodeAs(e, nv); err != nil {
return false, err
}
v = nv.Interface()
}
arr = append(arr, v)
}
dst.Set(reflect.ValueOf(arr))
return true, nil
}
return false, nil
}
Expand Down
57 changes: 53 additions & 4 deletions uast/types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
package uast
package uast_test

import (
"testing"

"reflect"
"testing"

"github.com/stretchr/testify/require"
. "gopkg.in/bblfsh/sdk.v2/uast"
"gopkg.in/bblfsh/sdk.v2/uast/nodes"
)

func init() {
RegisterPackage("test", arrayNode{})
}

var casesTypeOf = []struct {
name string
typ interface{}
Expand Down Expand Up @@ -38,6 +42,11 @@ func expPos(off int, line int, col int) nodes.Object {
}
}

type arrayNode struct {
GenNode
Array []Any `json:"Array"`
}

var casesToNode = []struct {
name string
obj interface{}
Expand Down Expand Up @@ -173,6 +182,46 @@ var casesToNode = []struct {
},
},
},
{
name: "arrayNode",
obj: arrayNode{
GenNode: GenNode{
Positions: Positions{
KeyStart: {Offset: 3, Line: 2, Col: 1},
KeyEnd: {Offset: 8, Line: 2, Col: 6},
},
},
Array: []Any{
Identifier{Name: "a", GenNode: GenNode{
Positions: Positions{},
}},
String{Value: "a", GenNode: GenNode{
Positions: Positions{},
}},
},
},
exp: nodes.Object{
KeyType: nodes.String("test:arrayNode"),
KeyPos: nodes.Object{
KeyType: nodes.String(TypePositions),
KeyStart: expPos(3, 2, 1),
KeyEnd: expPos(8, 2, 6),
},
"Array": nodes.Array{
nodes.Object{
KeyType: nodes.String("uast:Identifier"),
KeyPos: nodes.Object{KeyType: nodes.String(TypePositions)},
"Name": nodes.String("a"),
},
nodes.Object{
KeyType: nodes.String("uast:String"),
KeyPos: nodes.Object{KeyType: nodes.String(TypePositions)},
"Value": nodes.String("a"),
"Format": nodes.String(""),
},
},
},
},
}

func TestToNode(t *testing.T) {
Expand All @@ -183,7 +232,7 @@ func TestToNode(t *testing.T) {
require.Equal(t, c.exp, got)

nv := reflect.New(reflect.TypeOf(c.obj)).Elem()
err = nodeAs(got, nv)
err = NodeAs(got, nv)
require.NoError(t, err)
expObj := c.expObj
if expObj == nil {
Expand Down