code review 5824053: reflect: document PkgPath, Method, StructField

reflect: document PkgPath, Method, StructField

[rsc, 2012-03-14 19:04:19]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2012-03-14 19:06:37]

LGTM

On Wed, Mar 14, 2012 at 12:04 PM, <rsc@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> reflect: document PkgPath, Method, StructField
>
> Please review this at
http://codereview.appspot.com/**5824053/<http://codereview.appspot.com/5824053/>
>
> Affected files:
>  M src/pkg/reflect/type.go
>
>
> Index: src/pkg/reflect/type.go
> ==============================**==============================**=======
> --- a/src/pkg/reflect/type.go
> +++ b/src/pkg/reflect/type.go
> @@ -68,7 +68,8 @@
>
>        // PkgPath returns the type's package path.
>        // The package path is a full package import path like
> "encoding/base64".
> -       // PkgPath returns an empty string for unnamed or predeclared
> types.
> +       // PkgPath returns an empty string for unnamed types, like *T or
> struct{},
> +       // and for predeclared types, like error or string.
>        PkgPath() string
>
>        // Size returns the number of bytes needed to store
> @@ -354,11 +355,15 @@
>
>  // Method represents a single method.
>  type Method struct {
> -       PkgPath string // empty for uppercase Name
> -       Name    string
> -       Type    Type
> -       Func    Value
> -       Index   int
> +       // PkgPath is the package path that qualifies
> +       // a lower case (unexported) name.
> +       // It is empty for upper case (exported) names.
> +       PkgPath string
> +
> +       Name  string // method name
> +       Type  Type   // method type
> +       Func  Value  // func with receiver as first argument
> +       Index int    // index for Type.Method
>  }
>
>  // High bit says whether type has
> @@ -697,14 +702,19 @@
>        return
>  }
>
> +// A StructField describes a single field in a struct.
>  type StructField struct {
> -       PkgPath   string // empty for uppercase Name
> -       Name      string
> -       Type      Type
> -       Tag       StructTag
> -       Offset    uintptr
> -       Index     []int
> -       Anonymous bool
> +       // PkgPath is the package path that qualifies
> +       // a lower case (unexported) name.
> +       // It is empty for upper case (exported) names.
> +       PkgPath string
> +
> +       Name      string    // field name
> +       Type      Type      // field type
> +       Tag       StructTag // field tag string
> +       Offset    uintptr   // offset within struct, in bytes
> +       Index     []int     // index sequence for Type.FieldByIndex
> +       Anonymous bool      // is an anonymous field
>  }
>
>  // A StructTag is the tag string in a struct field.
>
>
>

[r, 2012-03-14 20:51:10]

http://codereview.appspot.com/5824053/diff/3/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):

http://codereview.appspot.com/5824053/diff/3/src/pkg/reflect/type.go#newcode70
src/pkg/reflect/type.go:70: // The package path is a full package import path
like "encoding/base64".
s/ like/: / would fix the grammar error here.

but see below

http://codereview.appspot.com/5824053/diff/3/src/pkg/reflect/type.go#newcode71
src/pkg/reflect/type.go:71: // PkgPath returns an empty string for unnamed
types, like *T or struct{},
s/, like/ such as/g

http://codereview.appspot.com/5824053/diff/3/src/pkg/reflect/type.go#newcode72
src/pkg/reflect/type.go:72: // and for predeclared types, like error or string.
here's a rewrite.

PkgPath returns the type's package path, that is, the
path string used to import the package, such as "encoding/base64".
If the type was predeclared (string, error) rather than imported,
the package path will be the empty string. It will also be empty
for unnamed types (*T, struct{}, []int, and so on).

not sure if you want, but rewritten this way i felt like finishing

for unnamed types (*T, struct{}, []int, and so on), since they
are constructed rather than imported.

since it seems to be true and explains things, but i'm not sure if
that's just superstition.

http://codereview.appspot.com/5824053/diff/3/src/pkg/reflect/type.go#newcode359
src/pkg/reflect/type.go:359: // a lower case (unexported) name.
needs an example or something. is the path for the method or the type? what
about the name? baffling.

i think at least s/name/metho/ here and /field/ in StructField but more is
needed.

[rsc, 2012-03-15 19:08:36]

http://codereview.appspot.com/5824053/diff/3/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):

http://codereview.appspot.com/5824053/diff/3/src/pkg/reflect/type.go#newcode72
src/pkg/reflect/type.go:72: // and for predeclared types, like error or string.
	// PkgPath returns a named type's package path, that is, the import path
	// that uniquely identifies the package, such as "encoding/base64".
	// If the type was predeclared (string, error) or unnamed (*T, struct{},
[]int),
	// the package path will be the empty string.
	PkgPath() string

http://codereview.appspot.com/5824053/diff/3/src/pkg/reflect/type.go#newcode359
src/pkg/reflect/type.go:359: // a lower case (unexported) name.
	// Name is the method name.
	// PkgPath is the package path that qualifies a lower case (unexported)
	// method name.  It is empty for upper case (exported) method names.
	// The combination of PkgPath and Name uniquely identifies a method
	// in a method set. 
	// See http://golang.org/ref/spec#Uniqueness_of_identifiers
	Name  string
	PkgPath string

(For now you have to insert tip. into that URL)

http://codereview.appspot.com/5824053/diff/3/src/pkg/reflect/type.go#newcode707
src/pkg/reflect/type.go:707: // PkgPath is the package path that qualifies
	// Name is the field name.
	// PkgPath is the package path that qualifies a lower case (unexported)
	// field name.  It is empty for upper case (exported) field names.
	// See http://golang.org/ref/spec#Uniqueness_of_identifiers
	Name  string
	PkgPath string

[r, 2012-03-15 19:38:52]

LGTM
thank you

[rsc, 2012-03-15 21:16:02]

*** Submitted as http://code.google.com/p/go/source/detail?r=526b4b12be9d ***

reflect: document PkgPath, Method, StructField

R=golang-dev, bradfitz, r
CC=golang-dev
http://codereview.appspot.com/5824053