Don't pretend *parser.List is immutable
The functions in bpfix that take a *parser.List and return a
modified *parser.List are always returning the same pointer
and mutating the target of the pointer. Remove the extra
unnecessary return value.
Also extract the getLiteralListProperty function.
Test: androidmk_test.go
Change-Id: I08d8aff955c72b7916741cda8083974a49af4d6f
diff --git a/bpfix/bpfix/bpfix.go b/bpfix/bpfix/bpfix.go
index fcd4fec..67a5909 100644
--- a/bpfix/bpfix/bpfix.go
+++ b/bpfix/bpfix/bpfix.go
@@ -41,20 +41,19 @@
// FixTree repeatedly applies the fixes listed in the given FixRequest to the given File
// until there is no fix that affects the tree
-func FixTree(tree *parser.File, config FixRequest) (fixed *parser.File, err error) {
+func FixTree(tree *parser.File, config FixRequest) error {
prevIdentifier, err := fingerprint(tree)
if err != nil {
- return nil, err
+ return err
}
- fixed = tree
maxNumIterations := 20
i := 0
for {
- fixed, err = fixTreeOnce(fixed, config)
+ err = fixTreeOnce(tree, config)
newIdentifier, err := fingerprint(tree)
if err != nil {
- return nil, err
+ return err
}
if bytes.Equal(newIdentifier, prevIdentifier) {
break
@@ -65,11 +64,11 @@
// detect infinite loop
i++
if i >= maxNumIterations {
- return nil, fmt.Errorf("Applied fixes %s times and yet the tree continued to change. Is there an infinite loop?", i)
+ return fmt.Errorf("Applied fixes %s times and yet the tree continued to change. Is there an infinite loop?", i)
break
}
}
- return fixed, err
+ return err
}
// returns a unique identifier for the given tree that can be used to determine whether the tree changed
@@ -81,20 +80,19 @@
return bytes, nil
}
-func fixTreeOnce(tree *parser.File, config FixRequest) (fixed *parser.File, err error) {
+func fixTreeOnce(tree *parser.File, config FixRequest) error {
if config.simplifyKnownRedundantVariables {
- tree, err = simplifyKnownPropertiesDuplicatingEachOther(tree)
+ err := simplifyKnownPropertiesDuplicatingEachOther(tree)
if err != nil {
- return nil, err
+ return err
}
}
- return tree, err
+ return nil
}
-func simplifyKnownPropertiesDuplicatingEachOther(tree *parser.File) (fixed *parser.File, err error) {
+func simplifyKnownPropertiesDuplicatingEachOther(tree *parser.File) error {
// remove from local_include_dirs anything in export_include_dirs
- fixed, err = removeMatchingModuleListProperties(tree, "export_include_dirs", "local_include_dirs")
- return fixed, err
+ return removeMatchingModuleListProperties(tree, "export_include_dirs", "local_include_dirs")
}
// removes from <items> every item present in <removals>
@@ -121,29 +119,30 @@
}
// Remove each modules[i].Properties[<legacyName>][j] that matches a modules[i].Properties[<canonicalName>][k]
-func removeMatchingModuleListProperties(tree *parser.File, canonicalName string, legacyName string) (fixed *parser.File, err error) {
+func removeMatchingModuleListProperties(tree *parser.File, canonicalName string, legacyName string) error {
for _, def := range tree.Defs {
mod, ok := def.(*parser.Module)
if !ok {
continue
}
- legacy, ok := mod.GetProperty(legacyName)
+ legacyList, ok := getLiteralListProperty(mod, legacyName)
if !ok {
continue
}
- legacyList, ok := legacy.Value.(*parser.List)
- if !ok {
- continue
- }
- canonical, ok := mod.GetProperty(canonicalName)
- if !ok {
- continue
- }
- canonicalList, ok := canonical.Value.(*parser.List)
+ canonicalList, ok := getLiteralListProperty(mod, canonicalName)
if !ok {
continue
}
filterExpressionList(legacyList, canonicalList)
}
- return tree, nil
+ return nil
+}
+
+func getLiteralListProperty(mod *parser.Module, name string) (list *parser.List, found bool) {
+ prop, ok := mod.GetProperty(name)
+ if !ok {
+ return nil, false
+ }
+ list, ok = prop.Value.(*parser.List)
+ return list, ok
}
diff --git a/bpfix/bpfix/bpfix_test.go b/bpfix/bpfix/bpfix_test.go
index 06ae139..e17e815 100644
--- a/bpfix/bpfix/bpfix_test.go
+++ b/bpfix/bpfix/bpfix_test.go
@@ -21,8 +21,9 @@
"strings"
"testing"
- "github.com/google/blueprint/parser"
"reflect"
+
+ "github.com/google/blueprint/parser"
)
// TODO(jeffrygaston) remove this when position is removed from ParseNode (in b/38325146) and we can directly do reflect.DeepEqual
@@ -62,7 +63,7 @@
}
// apply simplifications
- tree, err := simplifyKnownPropertiesDuplicatingEachOther(tree)
+ err := simplifyKnownPropertiesDuplicatingEachOther(tree)
if len(errs) > 0 {
t.Fatal(err)
}
diff --git a/bpfix/cmd/bpfix.go b/bpfix/cmd/bpfix.go
index f51c6f7..461f41d 100644
--- a/bpfix/cmd/bpfix.go
+++ b/bpfix/cmd/bpfix.go
@@ -75,13 +75,13 @@
}
// compute and apply any requested fixes
- fixed, err := bpfix.FixTree(file, fixRequest)
+ err = bpfix.FixTree(file, fixRequest)
if err != nil {
return err
}
// output the results
- res, err := parser.Print(fixed)
+ res, err := parser.Print(file)
if err != nil {
return err
}