From 969c70342ae4e47893226d47865abc6fc5a3cc2f Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Tue, 10 Mar 2015 14:37:27 -0700 Subject: [PATCH 1/3] Fix whitespace in parser/parser.go Change-Id: I75875cbf60efc9aaf7c2df5709533c2c04b6fba4 --- parser/parser.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/parser/parser.go b/parser/parser.go index 1716e03..0031547 100644 --- a/parser/parser.go +++ b/parser/parser.go @@ -321,11 +321,11 @@ func evaluateOperator(value1, value2 Value, operator rune, pos scanner.Position) func addMaps(map1, map2 []*Property, pos scanner.Position) ([]*Property, error) { ret := make([]*Property, 0, len(map1)) - + inMap1 := make(map[string]*Property) inMap2 := make(map[string]*Property) inBoth := make(map[string]*Property) - + for _, prop1 := range map1 { inMap1[prop1.Name.Name] = prop1 } @@ -336,7 +336,7 @@ func addMaps(map1, map2 []*Property, pos scanner.Position) ([]*Property, error) inBoth[prop2.Name.Name] = prop2 } } - + for _, prop1 := range map1 { if prop2, ok := inBoth[prop1.Name.Name]; ok { var err error @@ -356,7 +356,7 @@ func addMaps(map1, map2 []*Property, pos scanner.Position) ([]*Property, error) ret = append(ret, prop2) } } - + return ret, nil } From 62e681a288e8a047ccd5d03f676c7655731c9a7c Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Wed, 11 Mar 2015 14:36:03 -0700 Subject: [PATCH 2/3] Fix bug when copying slice to itself If proptools.CopyProperties is passed two values that point same slice then setting the destination slice to a new slice will overwrite the source slice, and the properties struct that is both the source and destination will have an empty slice. Copy into the new slice using a new reflect.Value, and then update the destination. Change-Id: I1bfcdc51e4278ea7c7ed81dafc928a5471219f05 --- proptools/proptools.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/proptools/proptools.go b/proptools/proptools.go index f490ba9..4868408 100644 --- a/proptools/proptools.go +++ b/proptools/proptools.go @@ -64,10 +64,12 @@ func CopyProperties(dstValue, srcValue reflect.Value) { panic(fmt.Errorf("can't copy field %q: slice elements are "+ "not strings", field.Name)) } - newSlice := reflect.MakeSlice(field.Type, srcFieldValue.Len(), - srcFieldValue.Len()) - dstFieldValue.Set(newSlice) - reflect.Copy(dstFieldValue, srcFieldValue) + if srcFieldValue != dstFieldValue { + newSlice := reflect.MakeSlice(field.Type, srcFieldValue.Len(), + srcFieldValue.Len()) + reflect.Copy(newSlice, srcFieldValue) + dstFieldValue.Set(newSlice) + } } else { dstFieldValue.Set(srcFieldValue) } From 10b54db7cc3294726ea7324ab63efc85858e3725 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Wed, 11 Mar 2015 14:40:30 -0700 Subject: [PATCH 3/3] Fix panic when dependency cycle includes the first module visited The cycle check can panic if the first call to check happens to land on the first module in a cycle. Print the cycle instead of panicking. Change-Id: I6fc1c66dcc37b1eb6b11b9e65343452af3c8538d --- context.go | 47 +++++++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/context.go b/context.go index c2f6e11..0062e36 100644 --- a/context.go +++ b/context.go @@ -1024,6 +1024,30 @@ func (c *Context) updateDependencies() (errs []error) { var check func(group *moduleGroup) []*moduleGroup + cycleError := func(cycle []*moduleGroup) { + // We are the "start" of the cycle, so we're responsible + // for generating the errors. The cycle list is in + // reverse order because all the 'check' calls append + // their own module to the list. + errs = append(errs, &Error{ + Err: fmt.Errorf("encountered dependency cycle:"), + Pos: cycle[len(cycle)-1].pos, + }) + + // Iterate backwards through the cycle list. + curGroup := cycle[len(cycle)-1] + for i := len(cycle) - 1; i >= 0; i-- { + nextGroup := cycle[i] + errs = append(errs, &Error{ + Err: fmt.Errorf(" %q depends on %q", + curGroup.properties.Name, + nextGroup.properties.Name), + Pos: curGroup.propertyPos["deps"], + }) + curGroup = nextGroup + } + } + check = func(group *moduleGroup) []*moduleGroup { visited[group] = true checking[group] = true @@ -1053,23 +1077,7 @@ func (c *Context) updateDependencies() (errs []error) { // for generating the errors. The cycle list is in // reverse order because all the 'check' calls append // their own module to the list. - errs = append(errs, &Error{ - Err: fmt.Errorf("encountered dependency cycle:"), - Pos: group.pos, - }) - - // Iterate backwards through the cycle list. - curGroup := group - for i := len(cycle) - 1; i >= 0; i-- { - nextGroup := cycle[i] - errs = append(errs, &Error{ - Err: fmt.Errorf(" %q depends on %q", - curGroup.properties.Name, - nextGroup.properties.Name), - Pos: curGroup.propertyPos["deps"], - }) - curGroup = nextGroup - } + cycleError(cycle) // We can continue processing this module's children to // find more cycles. Since all the modules that were @@ -1095,7 +1103,10 @@ func (c *Context) updateDependencies() (errs []error) { if !visited[group] { cycle := check(group) if cycle != nil { - panic("inconceivable!") + if cycle[len(cycle)-1] != group { + panic("inconceivable!") + } + cycleError(cycle) } } }