From 990e87d80d9989dd2fbac4db21c8527e1f17cea3 Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Fri, 3 May 2019 16:32:16 -0700 Subject: [PATCH] no need to initialize nil structs during path traversal --- parse.go | 47 +++++++++++------------------------------------ parse_test.go | 20 ++++++++++++++++++++ subcommand.go | 2 +- usage.go | 2 +- 4 files changed, 33 insertions(+), 38 deletions(-) diff --git a/parse.go b/parse.go index e0de705..2126ee5 100644 --- a/parse.go +++ b/parse.go @@ -393,7 +393,7 @@ func (p *Parser) captureEnvVars(specs []*spec, wasPresent map[*spec]bool) error err, ) } - if err = setSlice(p.writable(spec.dest), values, !spec.separate); err != nil { + if err = setSlice(p.val(spec.dest), values, !spec.separate); err != nil { return fmt.Errorf( "error processing environment variable %s with multiple values: %v", spec.env, @@ -401,7 +401,7 @@ func (p *Parser) captureEnvVars(specs []*spec, wasPresent map[*spec]bool) error ) } } else { - if err := scalar.ParseValue(p.writable(spec.dest), value); err != nil { + if err := scalar.ParseValue(p.val(spec.dest), value); err != nil { return fmt.Errorf("error processing environment variable %s: %v", spec.env, err) } } @@ -457,7 +457,7 @@ func (p *Parser) process(args []string) error { } // instantiate the field to point to a new struct - v := p.writable(subcmd.dest) + v := p.val(subcmd.dest) v.Set(reflect.New(v.Type().Elem())) // we already checked that all subcommands are struct pointers // add the new options to the set of allowed options @@ -512,7 +512,7 @@ func (p *Parser) process(args []string) error { } else { values = append(values, value) } - err := setSlice(p.writable(spec.dest), values, !spec.separate) + err := setSlice(p.val(spec.dest), values, !spec.separate) if err != nil { return fmt.Errorf("error processing %s: %v", arg, err) } @@ -537,7 +537,7 @@ func (p *Parser) process(args []string) error { i++ } - err := scalar.ParseValue(p.writable(spec.dest), value) + err := scalar.ParseValue(p.val(spec.dest), value) if err != nil { return fmt.Errorf("error processing %s: %v", arg, err) } @@ -553,13 +553,13 @@ func (p *Parser) process(args []string) error { } wasPresent[spec] = true if spec.multiple { - err := setSlice(p.writable(spec.dest), positionals, true) + err := setSlice(p.val(spec.dest), positionals, true) if err != nil { return fmt.Errorf("error processing %s: %v", spec.long, err) } positionals = nil } else { - err := scalar.ParseValue(p.writable(spec.dest), positionals[0]) + err := scalar.ParseValue(p.val(spec.dest), positionals[0]) if err != nil { return fmt.Errorf("error processing %s: %v", spec.long, err) } @@ -602,9 +602,9 @@ func isFlag(s string) bool { return strings.HasPrefix(s, "-") && strings.TrimLeft(s, "-") != "" } -// readable returns a reflect.Value corresponding to the current value for the -// given -func (p *Parser) readable(dest path) reflect.Value { +// val returns a reflect.Value corresponding to the current value for the +// given path +func (p *Parser) val(dest path) reflect.Value { v := p.roots[dest.root] for _, field := range dest.fields { if v.Kind() == reflect.Ptr { @@ -626,35 +626,10 @@ func (p *Parser) readable(dest path) reflect.Value { return v } -// writable trav.patherses the destination struct to find the destination to -// which the value of the given spec should be written. It fills in null -// structs with pointers to the zero value for that struct. -func (p *Parser) writable(dest path) reflect.Value { - v := p.roots[dest.root] - for _, field := range dest.fields { - if v.Kind() == reflect.Ptr { - if v.IsNil() { - v.Set(reflect.New(v.Type().Elem())) - } - v = v.Elem() - } - - v = v.FieldByName(field) - if !v.IsValid() { - // it is appropriate to panic here because this can only happen due to - // an internal bug in this library (since we construct the path ourselves - // by reflecting on the same struct) - panic(fmt.Errorf("error resolving path %v: %v has no field named %v", - dest.fields, v.Type(), field)) - } - } - return v -} - // parse a value as the appropriate type and store it in the struct func setSlice(dest reflect.Value, values []string, trunc bool) error { if !dest.CanSet() { - return fmt.Errorf("field is not writable") + return fmt.Errorf("field is not val") } var ptr bool diff --git a/parse_test.go b/parse_test.go index c2544fd..c18dc16 100644 --- a/parse_test.go +++ b/parse_test.go @@ -877,6 +877,26 @@ func TestEmbedded(t *testing.T) { assert.Equal(t, true, args.Z) } +func TestEmbeddedPtr(t *testing.T) { + // embedded pointer fields are not supported so this should return an error + var args struct { + *A + } + err := parse("--x=hello", &args) + require.Error(t, err) +} + +func TestEmbeddedPtrIgnored(t *testing.T) { + // embedded pointer fields are not supported but here we + var args struct { + *A `arg:"-"` + B + } + err := parse("--y=321", &args) + require.NoError(t, err) + assert.Equal(t, 321, args.Y) +} + func TestEmptyArgs(t *testing.T) { origArgs := os.Args diff --git a/subcommand.go b/subcommand.go index b73e933..dff732c 100644 --- a/subcommand.go +++ b/subcommand.go @@ -10,7 +10,7 @@ func (p *Parser) Subcommand() interface{} { if p.lastCmd == nil || p.lastCmd.parent == nil { return nil } - return p.readable(p.lastCmd.dest).Interface() + return p.val(p.lastCmd.dest).Interface() } // SubcommandNames returns the sequence of subcommands specified by the diff --git a/usage.go b/usage.go index 1f6b569..33b6184 100644 --- a/usage.go +++ b/usage.go @@ -179,7 +179,7 @@ func (p *Parser) printOption(w io.Writer, spec *spec) { // If spec.dest is not the zero value then a default value has been added. var v reflect.Value if len(spec.dest.fields) > 0 { - v = p.readable(spec.dest) + v = p.val(spec.dest) } var defaultVal *string