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

androidmk/cmd/androidmk/androidmk.go

@@ -239,12 +239,12 @@ func convertFile(filename string, buffer *bytes.Buffer) (string, []error) {
 	}
 
 	// check for common supported but undesirable structures and clean them up
-	fixed, err := bpfix.FixTree(tree, bpfix.NewFixRequest().AddAll())
+	err := bpfix.FixTree(tree, bpfix.NewFixRequest().AddAll())
 	if err != nil {
 		return "", []error{err}
 	}
 
-	out, err := bpparser.Print(fixed)
+	out, err := bpparser.Print(tree)
 	if err != nil {
 		return "", []error{err}
 	}

bpfix/bpfix/bpfix.go

@@ -41,20 +41,19 @@ func (r FixRequest) AddAll() (result FixRequest) {
 
 // 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 @@ func FixTree(tree *parser.File, config FixRequest) (fixed *parser.File, err erro
 		// 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 @@ func fingerprint(tree *parser.File) (fingerprint []byte, err error) {
 	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 removeMatchingModuleListProperties(tree, "export_include_dirs", "local_include_dirs")
-	return fixed, err
 }
 
 // removes from <items> every item present in <removals>
@@ -121,29 +119,30 @@ func filterExpressionList(items *parser.List, removals *parser.List) {
 }
 
 // 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)
+		canonicalList, ok := getLiteralListProperty(mod, canonicalName)
-		if !ok {
-			continue
-		}
-		canonical, ok := mod.GetProperty(canonicalName)
-		if !ok {
-			continue
-		}
-		canonicalList, ok := canonical.Value.(*parser.List)
 		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
 }

bpfix/bpfix/bpfix_test.go

@@ -21,8 +21,9 @@ import (
 	"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 @@ func implFilterListTest(t *testing.T, local_include_dirs []string, export_includ
 	}
 
 	// apply simplifications
-	tree, err := simplifyKnownPropertiesDuplicatingEachOther(tree)
+	err := simplifyKnownPropertiesDuplicatingEachOther(tree)
 	if len(errs) > 0 {
 		t.Fatal(err)
 	}

bpfix/cmd/bpfix.go

@@ -75,13 +75,13 @@ func processFile(filename string, in io.Reader, out io.Writer, fixRequest bpfix.
 	}
 
 	// 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
 	}