Merge pull request #1224 from keleustes/varset

Change the backing data structure of VarSet from slice to map

pkg/accumulator/resaccumulator.go

@@ -28,7 +28,7 @@ func MakeEmptyAccumulator() *ResAccumulator {
 	ra := &ResAccumulator{}
 	ra.resMap = resmap.New()
 	ra.tConfig = &config.TransformerConfig{}
-	ra.varSet = types.VarSet{}
+	ra.varSet = types.NewVarSet()
 	return ra
 }
 

pkg/types/var.go

@@ -70,26 +70,45 @@ func (v *Var) defaulting() {
 	}
 }
 
-// VarSet is a slice of Vars where no var.Name is repeated.
+// VarSet is a set of Vars where no var.Name is repeated.
 type VarSet struct {
-	set []Var
+	set map[string]Var
+}
+
+// NewVarSet returns an initialized VarSet
+func NewVarSet() VarSet {
+	return VarSet{set: map[string]Var{}}
 }
 
 // AsSlice returns the vars as a slice.
 func (vs *VarSet) AsSlice() []Var {
 	s := make([]Var, len(vs.set))
-	copy(s, vs.set)
+	i := 0
+	for _, v := range vs.set {
+		s[i] = v
+		i++
+	}
+	sort.Sort(ByName(s))
 	return s
 }
 
 // Copy returns a copy of the var set.
 func (vs *VarSet) Copy() VarSet {
-	return VarSet{set: vs.AsSlice()}
+	newSet := make(map[string]Var, len(vs.set))
+	for k, v := range vs.set {
+		newSet[k] = v
+	}
+	return VarSet{set: newSet}
 }
 
 // MergeSet absorbs other vars with error on name collision.
 func (vs *VarSet) MergeSet(incoming VarSet) error {
-	return vs.MergeSlice(incoming.set)
+	for _, incomingVar := range incoming.set {
+		if err := vs.Merge(incomingVar); err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
 // MergeSlice absorbs a Var slice with error on name collision.
@@ -111,33 +130,19 @@ func (vs *VarSet) Merge(v Var) error {
 			"var '%s' already encountered", v.Name)
 	}
 	v.defaulting()
-	vs.insert(v)
+	vs.set[v.Name] = v
 	return nil
 }
 
-func (vs *VarSet) insert(v Var) {
-	index := sort.Search(
-		len(vs.set),
-		func(i int) bool { return vs.set[i].Name > v.Name })
-	// make room
-	vs.set = append(vs.set, Var{})
-	// shift right at index.
-	// copy will not increase size of destination.
-	copy(vs.set[index+1:], vs.set[index:])
-	vs.set[index] = v
-}
-
 // Contains is true if the set has the other var.
 func (vs *VarSet) Contains(other Var) bool {
 	return vs.Get(other.Name) != nil
 }
 
 // Get returns the var with the given name, else nil.
 func (vs *VarSet) Get(name string) *Var {
-	for _, v := range vs.set {
-		if v.Name == name {
-			return &v
-		}
+	if v, found := vs.set[name]; found {
+		return &v
 	}
 	return nil
 }
@@ -157,3 +162,10 @@ func (t *Target) GVK() gvk.Gvk {
 	}
 	return t.Gvk
 }
+
+// ByName is a sort interface which sorts Vars by name alphabetically
+type ByName []Var
+
+func (v ByName) Len() int           { return len(v) }
+func (v ByName) Swap(i, j int)      { v[i], v[j] = v[j], v[i] }
+func (v ByName) Less(i, j int) bool { return v[i].Name < v[j].Name }

pkg/types/var_test.go

@@ -89,7 +89,7 @@ func TestDefaulting(t *testing.T) {
 }
 
 func TestVarSet(t *testing.T) {
-	set := VarSet{}
+	set := NewVarSet()
 	vars := []Var{
 		{
 			Name: "SHELLVARS",
@@ -123,7 +123,7 @@ func TestVarSet(t *testing.T) {
 			t.Fatalf("set %v should contain var %v", set.AsSlice(), v)
 		}
 	}
-	set2 := VarSet{}
+	set2 := NewVarSet()
 	err = set2.MergeSet(set)
 	if err != nil {
 		t.Fatalf("unexpected err: %v", err)
@@ -151,3 +151,34 @@ func TestVarSet(t *testing.T) {
 		t.Fatalf("unexpected order in : %v", names)
 	}
 }
+
+func TestVarSetCopy(t *testing.T) {
+	set1 := NewVarSet()
+	vars := []Var{
+		{Name: "First"},
+		{Name: "Second"},
+		{Name: "Third"},
+	}
+	err := set1.MergeSlice(vars)
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	// Confirm copying
+	set2 := set1.Copy()
+	for _, varInSet1 := range set1.AsSlice() {
+		if v := set2.Get(varInSet1.Name); v == nil {
+			t.Fatalf("set %v should contain a Var named %s", set2.AsSlice(), varInSet1)
+		} else if !set2.Contains(*v) {
+			t.Fatalf("set %v should contain %v", set2.AsSlice(), v)
+		}
+	}
+	// Confirm that the copy is deep
+	w := Var{Name: "Only in set2"}
+	set2.Merge(w)
+	if !set2.Contains(w) {
+		t.Fatalf("set %v should contain %v", set2.AsSlice(), w)
+	}
+	if set1.Contains(w) {
+		t.Fatalf("set %v should not contain %v", set1.AsSlice(), w)
+	}
+}