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

Use Go 1.20 slices package #738

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion engine/graph/actions.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package graph
import (
"context"
"fmt"
"slices"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -76,7 +77,7 @@ func (obj *Engine) Process(ctx context.Context, vertex pgraph.Vertex) error {
// back poke in parallel (sync b/c of waitgroup)
wg := &sync.WaitGroup{}
for _, v := range obj.graph.IncomingGraphVertices(vertex) {
if !pgraph.VertexContains(v, vs) { // only poke what's needed
if !slices.Contains(vs, v) { // only poke what's needed
continue
}

Expand Down
5 changes: 3 additions & 2 deletions engine/graph/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"fmt"
"os"
"path"
"slices"
"sync"

"github.com/purpleidea/mgmt/converger"
Expand Down Expand Up @@ -420,9 +421,9 @@ func (obj *Engine) Resume() error {
return err
}
//indegree := obj.graph.InDegree() // compute all of the indegree's
reversed := pgraph.Reverse(topoSort)
slices.Reverse(topoSort)

for _, vertex := range reversed {
for _, vertex := range topoSort {
// The very first resume is skipped as those resources are
// already running! We could do that by checking here, but it is
// more convenient to just have a state struct field (paused) to
Expand Down
5 changes: 3 additions & 2 deletions lang/funcs/dage/dage.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"context"
"fmt"
"os"
"slices"
"sort"
"strings"
"sync"
Expand Down Expand Up @@ -1185,8 +1186,8 @@ func (obj *Engine) Run(ctx context.Context) (reterr error) {
if err != nil {
return err
}
reversed := pgraph.Reverse(topoSort2)
for _, v := range reversed {
slices.Reverse(topoSort2)
for _, v := range topoSort2 {
f, ok := v.(interfaces.Func)
if !ok {
panic("not a Func")
Expand Down
15 changes: 3 additions & 12 deletions lang/unification/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
package unification

import (
"slices"

"github.com/purpleidea/mgmt/lang/interfaces"
)

Expand Down Expand Up @@ -53,17 +55,6 @@ func UniqueExprList(exprList []interfaces.Expr) []interfaces.Expr {
return ExprMapToExprList(exprMap)
}

// ExprContains is an "in array" function to test for an expr in a slice of
// expressions.
func ExprContains(needle interfaces.Expr, haystack []interfaces.Expr) bool {
for _, v := range haystack {
if needle == v {
return true
}
}
return false
}

// pairs is a simple list of pairs of expressions which can be used as a simple
// undirected graph structure, or as a simple list of equalities.
type pairs []*interfaces.EqualityInvariant
Expand Down Expand Up @@ -112,7 +103,7 @@ func (obj pairs) DFS(start interfaces.Expr) []interfaces.Expr {
for len(s) > 0 {
v, s = s[len(s)-1], s[:len(s)-1] // s.pop()

if !ExprContains(v, d) { // if not discovered
if !slices.Contains(d, v) { // if not discovered
d = append(d, v) // label as discovered

for _, w := range obj.Vertices(v) {
Expand Down
5 changes: 3 additions & 2 deletions pgraph/graphsync.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package pgraph

import (
"fmt"
"slices"

"github.com/purpleidea/mgmt/util/errwrap"
)
Expand Down Expand Up @@ -105,7 +106,7 @@ func (obj *Graph) GraphSync(newGraph *Graph, vertexCmpFn func(Vertex, Vertex) (b
}
// get rid of any vertices we shouldn't keep (that aren't in new graph)
for v := range oldGraph.Adjacency() {
if !VertexContains(v, vertexKeep) {
if !slices.Contains(vertexKeep, v) {
vertexDels = append(vertexDels, v) // append
}
}
Expand Down Expand Up @@ -163,7 +164,7 @@ func (obj *Graph) GraphSync(newGraph *Graph, vertexCmpFn func(Vertex, Vertex) (b
for v1 := range oldGraph.Adjacency() {
for _, e := range oldGraph.Adjacency()[v1] {
// we have an edge!
if !EdgeContains(e, edgeKeep) {
if !slices.Contains(edgeKeep, e) {
oldGraph.DeleteEdge(e)
}
}
Expand Down
65 changes: 11 additions & 54 deletions pgraph/pgraph.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ package pgraph
import (
"errors"
"fmt"
"slices"
"sort"
"strings"

Expand Down Expand Up @@ -362,11 +363,11 @@ func (vs VertexSlice) Sort() { sort.Sort(vs) }
// VerticesSorted returns a sorted slice of all vertices in the graph. The order
// is sorted by String() to avoid the non-determinism in the map type.
func (g *Graph) VerticesSorted() []Vertex {
var vertices []Vertex
var vertices VertexSlice
for k := range g.adjacency {
vertices = append(vertices, k)
}
sort.Sort(VertexSlice(vertices)) // add determinism
sort.Sort(vertices) // add determinism
return vertices
}

Expand All @@ -387,11 +388,11 @@ func (g *Graph) Sprint() string {
str += fmt.Sprintf("Vertex: %s\n", v)
}
for _, v1 := range g.VerticesSorted() {
vs := []Vertex{}
var vs VertexSlice
Copy link
Owner

Choose a reason for hiding this comment

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

Please leave these alone for now. Explicitly seeing the list is more logical at least for me.

for v2 := range g.Adjacency()[v1] {
vs = append(vs, v2)
}
sort.Sort(VertexSlice(vs)) // deterministic order
sort.Sort(vs) // deterministic order
for _, v2 := range vs {
e := g.Adjacency()[v1][v2]
str += fmt.Sprintf("Edge: %s -> %s # %s\n", v1, v2, e)
Expand Down Expand Up @@ -488,7 +489,7 @@ func (g *Graph) DFS(start Vertex) []Vertex {
for len(s) > 0 {
v, s = s[len(s)-1], s[:len(s)-1] // s.pop()

if !VertexContains(v, d) { // if not discovered
if !slices.Contains(d, v) { // if not discovered
d = append(d, v) // label as discovered

for _, w := range g.GraphVertices(v) {
Expand All @@ -502,7 +503,7 @@ func (g *Graph) DFS(start Vertex) []Vertex {
// FilterGraph builds a new graph containing only vertices from the list.
func (g *Graph) FilterGraph(vertices []Vertex) (*Graph, error) {
fn := func(v Vertex) (bool, error) {
return VertexContains(v, vertices), nil
return slices.Contains(vertices, v), nil
}
return g.FilterGraphWithFn(fn)
}
Expand Down Expand Up @@ -545,7 +546,7 @@ func (g *Graph) DisconnectedGraphs() ([]*Graph, error) {

// get an undiscovered vertex to start from
for _, s := range g.Vertices() {
if !VertexContains(s, d) {
if !slices.Contains(d, s) {
start = s
}
}
Expand Down Expand Up @@ -675,7 +676,7 @@ func (g *Graph) Reachability(a, b Vertex) ([]Vertex, error) {
if len(vertices) == 0 {
return []Vertex{}, nil // nope
}
if VertexContains(b, vertices) {
if slices.Contains(vertices, b) {
return []Vertex{a, b}, nil // found
}
// TODO: parallelize this with go routines?
Expand Down Expand Up @@ -764,10 +765,10 @@ Loop:
m1 := []Vertex{}
m2 := []Vertex{}
for k, v := range m {
if VertexContains(k, m1) {
if slices.Contains(m1, k) {
return fmt.Errorf("mapping from %s is used more than once to: %s", k, m1)
}
if VertexContains(v, m2) {
if slices.Contains(m2, v) {
return fmt.Errorf("mapping to %s is used more than once from: %s", v, m2)
}
m1 = append(m1, k)
Expand Down Expand Up @@ -804,47 +805,3 @@ Loop:

return nil // success!
}

// VertexContains is an "in array" function to test for a vertex in a slice of
// vertices.
func VertexContains(needle Vertex, haystack []Vertex) bool {
for _, v := range haystack {
if needle == v {
return true
}
}
return false
}

// EdgeContains is an "in array" function to test for an edge in a slice of
// edges.
func EdgeContains(needle Edge, haystack []Edge) bool {
for _, v := range haystack {
if needle == v {
return true
}
}
return false
}

// Reverse reverses a list of vertices.
func Reverse(vs []Vertex) []Vertex {
out := []Vertex{}
l := len(vs)
for i := range vs {
out = append(out, vs[l-i-1])
}
return out
}

// Sort the list of vertices and return a copy without modifying the input.
func Sort(vs []Vertex) []Vertex {
vertices := []Vertex{}
for _, v := range vs { // copy
vertices = append(vertices, v)
}
sort.Sort(VertexSlice(vertices))
return vertices
// sort.Sort(VertexSlice(vs)) // this is wrong, it would modify input!
//return vs
}
101 changes: 0 additions & 101 deletions pgraph/pgraph_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -323,34 +323,6 @@ func TestDeleteVertex2(t *testing.T) {
}
}

func TestVertexContains1(t *testing.T) {
v1 := NV("v1")
v2 := NV("v2")
v3 := NV("v3")
if VertexContains(v1, []Vertex{v1, v2, v3}) != true {
Copy link
Owner

Choose a reason for hiding this comment

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

You can leave all the tests in, let's just test the new methods you're using!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there much point in testing stdlib functionality? The point of this change was to reduce the amount of code we're carrying. Less code -> easier to understand/reason about

t.Errorf("should be true instead of false.")
}

v4 := NV("v4")
v5 := NV("v5")
v6 := NV("v6")
if VertexContains(v4, []Vertex{v5, v6}) != false {
t.Errorf("should be false instead of true.")
}

v7 := NV("v7")
v8 := NV("v8")
v9 := NV("v9")
if VertexContains(v8, []Vertex{v7, v8, v9}) != true {
t.Errorf("should be true instead of false.")
}

v1b := NV("v1") // same value, different objects
if VertexContains(v1b, []Vertex{v1, v2, v3}) != false {
t.Errorf("should be false instead of true.")
}
}

func TestTopoSort1(t *testing.T) {
G, _ := NewGraph("g9")
v1 := NV("v1")
Expand Down Expand Up @@ -683,31 +655,6 @@ func TestReachability4(t *testing.T) {
}
}

func TestReverse1(t *testing.T) {
v1 := NV("v1")
v2 := NV("v2")
v3 := NV("v3")
v4 := NV("v4")
v5 := NV("v5")
v6 := NV("v6")

if rev := Reverse([]Vertex{}); !reflect.DeepEqual(rev, []Vertex{}) {
t.Errorf("reverse of vertex slice failed (empty)")
}

if rev := Reverse([]Vertex{v1}); !reflect.DeepEqual(rev, []Vertex{v1}) {
t.Errorf("reverse of vertex slice failed (single)")
}

if rev := Reverse([]Vertex{v1, v2, v3, v4, v5, v6}); !reflect.DeepEqual(rev, []Vertex{v6, v5, v4, v3, v2, v1}) {
t.Errorf("reverse of vertex slice failed (1..6)")
}

if rev := Reverse([]Vertex{v6, v5, v4, v3, v2, v1}); !reflect.DeepEqual(rev, []Vertex{v1, v2, v3, v4, v5, v6}) {
t.Errorf("reverse of vertex slice failed (6..1)")
}
}

func TestCopy1(t *testing.T) {
g1 := &Graph{}
g2 := g1.Copy() // check this doesn't panic
Expand Down Expand Up @@ -750,54 +697,6 @@ func TestGraphCmp1(t *testing.T) {
// }
//}

func TestSort0(t *testing.T) {
vs := []Vertex{}
s := Sort(vs)

if !reflect.DeepEqual(s, []Vertex{}) {
t.Errorf("sort failed!")
if s == nil {
t.Logf("output is nil!")
} else {
str := "Got:"
for _, v := range s {
str += " " + v.String()
}
t.Errorf(str)
}
}
}

func TestSort1(t *testing.T) {
v1 := NV("v1")
v2 := NV("v2")
v3 := NV("v3")
v4 := NV("v4")
v5 := NV("v5")
v6 := NV("v6")

vs := []Vertex{v3, v2, v6, v1, v5, v4}
s := Sort(vs)

if !reflect.DeepEqual(s, []Vertex{v1, v2, v3, v4, v5, v6}) {
t.Errorf("sort failed!")
str := "Got:"
for _, v := range s {
str += " " + v.String()
}
t.Errorf(str)
}

if !reflect.DeepEqual(vs, []Vertex{v3, v2, v6, v1, v5, v4}) {
t.Errorf("sort modified input!")
str := "Got:"
for _, v := range vs {
str += " " + v.String()
}
t.Errorf(str)
}
}

func TestSprint1(t *testing.T) {
g, _ := NewGraph("graph1")
v1 := NV("v1")
Expand Down
Loading