From adee968a4bace28bd26253bc436043eb513f6117 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Wed, 11 Apr 2018 16:10:18 -0700 Subject: [PATCH] Make bpfix not modify the input tree Make a new object called Fixer to hold the state of the tree, and make a copy of the input tree so the original doesn't get modified. Test: bpfix_test.go Change-Id: I1dc6fd99158c8b0e1db029df99e6cf72699a5e63 --- androidmk/cmd/androidmk/androidmk.go | 3 +- bpfix/bpfix/bpfix.go | 80 +++++++++++++++++++++------- bpfix/bpfix/bpfix_test.go | 6 ++- bpfix/cmd/bpfix.go | 3 +- 4 files changed, 68 insertions(+), 24 deletions(-) diff --git a/androidmk/cmd/androidmk/androidmk.go b/androidmk/cmd/androidmk/androidmk.go index 6e0b474bc..b6a973c0d 100644 --- a/androidmk/cmd/androidmk/androidmk.go +++ b/androidmk/cmd/androidmk/androidmk.go @@ -239,7 +239,8 @@ func convertFile(filename string, buffer *bytes.Buffer) (string, []error) { } // check for common supported but undesirable structures and clean them up - err := bpfix.FixTree(tree, bpfix.NewFixRequest().AddAll()) + fixer := bpfix.NewFixer(tree) + tree, err := fixer.Fix(bpfix.NewFixRequest().AddAll()) if err != nil { return "", []error{err} } diff --git a/bpfix/bpfix/bpfix.go b/bpfix/bpfix/bpfix.go index 2c3cc6cdd..4300e81b6 100644 --- a/bpfix/bpfix/bpfix.go +++ b/bpfix/bpfix/bpfix.go @@ -18,7 +18,9 @@ package bpfix import ( "bytes" + "errors" "fmt" + "io" "path/filepath" "github.com/google/blueprint/parser" @@ -42,21 +44,34 @@ func (r FixRequest) AddAll() (result FixRequest) { return result } -// FixTree repeatedly applies the fixes listed in the given FixRequest to the given File +type Fixer struct { + tree *parser.File +} + +func NewFixer(tree *parser.File) *Fixer { + fixer := &Fixer{tree} + + // make a copy of the tree + fixer.reparse() + + return fixer +} + +// Fix 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) error { - prevIdentifier, err := fingerprint(tree) +func (f *Fixer) Fix(config FixRequest) (*parser.File, error) { + prevIdentifier, err := f.fingerprint() if err != nil { - return err + return nil, err } maxNumIterations := 20 i := 0 for { - err = fixTreeOnce(tree, config) - newIdentifier, err := fingerprint(tree) + err = f.fixTreeOnce(config) + newIdentifier, err := f.fingerprint() if err != nil { - return err + return nil, err } if bytes.Equal(newIdentifier, prevIdentifier) { break @@ -67,31 +82,56 @@ func FixTree(tree *parser.File, config FixRequest) error { // detect infinite loop i++ if i >= maxNumIterations { - return fmt.Errorf("Applied fixes %d times and yet the tree continued to change. Is there an infinite loop?", i) + return nil, fmt.Errorf("Applied fixes %d times and yet the tree continued to change. Is there an infinite loop?", i) break } } - return err + return f.tree, err } // returns a unique identifier for the given tree that can be used to determine whether the tree changed -func fingerprint(tree *parser.File) (fingerprint []byte, err error) { - bytes, err := parser.Print(tree) +func (f *Fixer) fingerprint() (fingerprint []byte, err error) { + bytes, err := parser.Print(f.tree) if err != nil { return nil, err } return bytes, nil } -func fixTreeOnce(tree *parser.File, config FixRequest) error { +func (f *Fixer) reparse() ([]byte, error) { + buf, err := parser.Print(f.tree) + if err != nil { + return nil, err + } + newTree, err := parse(f.tree.Name, bytes.NewReader(buf)) + if err != nil { + return nil, err + } + f.tree = newTree + return buf, nil +} + +func parse(name string, r io.Reader) (*parser.File, error) { + tree, errs := parser.Parse(name, r, parser.NewScope(nil)) + if errs != nil { + s := "parse error: " + for _, err := range errs { + s += "\n" + err.Error() + } + return nil, errors.New(s) + } + return tree, nil +} + +func (f *Fixer) fixTreeOnce(config FixRequest) error { if config.simplifyKnownRedundantVariables { - err := simplifyKnownPropertiesDuplicatingEachOther(tree) + err := f.simplifyKnownPropertiesDuplicatingEachOther() if err != nil { return err } } if config.rewriteIncorrectAndroidmkPrebuilts { - err := rewriteIncorrectAndroidmkPrebuilts(tree) + err := f.rewriteIncorrectAndroidmkPrebuilts() if err != nil { return err } @@ -99,13 +139,13 @@ func fixTreeOnce(tree *parser.File, config FixRequest) error { return nil } -func simplifyKnownPropertiesDuplicatingEachOther(tree *parser.File) error { +func (f *Fixer) simplifyKnownPropertiesDuplicatingEachOther() error { // remove from local_include_dirs anything in export_include_dirs - return removeMatchingModuleListProperties(tree, "export_include_dirs", "local_include_dirs") + return f.removeMatchingModuleListProperties("export_include_dirs", "local_include_dirs") } -func rewriteIncorrectAndroidmkPrebuilts(tree *parser.File) error { - for _, def := range tree.Defs { +func (f *Fixer) rewriteIncorrectAndroidmkPrebuilts() error { + for _, def := range f.tree.Defs { mod, ok := def.(*parser.Module) if !ok { continue @@ -163,8 +203,8 @@ func filterExpressionList(items *parser.List, removals *parser.List) { } // Remove each modules[i].Properties[][j] that matches a modules[i].Properties[][k] -func removeMatchingModuleListProperties(tree *parser.File, canonicalName string, legacyName string) error { - for _, def := range tree.Defs { +func (f *Fixer) removeMatchingModuleListProperties(canonicalName string, legacyName string) error { + for _, def := range f.tree.Defs { mod, ok := def.(*parser.Module) if !ok { continue diff --git a/bpfix/bpfix/bpfix_test.go b/bpfix/bpfix/bpfix_test.go index e17e815a0..1e1e69337 100644 --- a/bpfix/bpfix/bpfix_test.go +++ b/bpfix/bpfix/bpfix_test.go @@ -62,14 +62,16 @@ func implFilterListTest(t *testing.T, local_include_dirs []string, export_includ t.Fatalf("%d parse errors", len(errs)) } + fixer := NewFixer(tree) + // apply simplifications - err := simplifyKnownPropertiesDuplicatingEachOther(tree) + err := fixer.simplifyKnownPropertiesDuplicatingEachOther() if len(errs) > 0 { t.Fatal(err) } // lookup legacy property - mod := tree.Defs[0].(*parser.Module) + mod := fixer.tree.Defs[0].(*parser.Module) _, found := mod.GetProperty("local_include_dirs") if !found { t.Fatalf("failed to include key local_include_dirs in parse tree") diff --git a/bpfix/cmd/bpfix.go b/bpfix/cmd/bpfix.go index 461f41ddd..2fde3836f 100644 --- a/bpfix/cmd/bpfix.go +++ b/bpfix/cmd/bpfix.go @@ -75,7 +75,8 @@ func processFile(filename string, in io.Reader, out io.Writer, fixRequest bpfix. } // compute and apply any requested fixes - err = bpfix.FixTree(file, fixRequest) + fixer := bpfix.NewFixer(file) + file, err = fixer.Fix(fixRequest) if err != nil { return err }