code review 81360046: go.tools/cmd/godex: fix prefix generation, filtering, f...

go.tools/cmd/godex: fix prefix generation, filtering, formatting

Details:
- auto-generate prefixes for std lib (e.g., "godex big" works now)
- apply filtering to package-level objects only
- nicer formatting of single-entry const, var, or type declaration

[gri, 2014-03-27 23:41:40]

Hello adonovan@google.com (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.tools

[adonovan, 2014-03-28 15:53:59]

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go
File cmd/godex/print.go (right):

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode65
cmd/godex/print.go:65: if obj := scope.Lookup(name); obj.Exported() {
The least restrictive filter is now: Exported().
I think that makes sense since the maintainer of a package can look at the
source to find unexported names, but I just wanted to confirm the intent.

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode65
cmd/godex/print.go:65: if obj := scope.Lookup(name); obj.Exported() {
I would move the obj:= assignment out of the if-stmt since it's used in both
branches, and there's nothing following the if-stmt.

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode74
cmd/godex/print.go:74: if named, _ := obj.Type().(*types.Named); named != nil &&
named.NumMethods() > 0 {
named!=nil is a tautology here: how can a TypeName's type be anything other than
a Named?

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode89
cmd/godex/print.go:89: // no filtering: collect top-level unexported types with
methods
Why not apply filtering to unexported names too?

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode92
cmd/godex/print.go:92: if named, _ := obj.Type().(*types.Named); named != nil &&
named.NumMethods() > 0 {
ditto (named!=nil)

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode189
cmd/godex/print.go:189: // TODO(gri) use floating-point notation for exact
floating-point numbers (fractions)
Perhaps exact.Value should do that?  I've wanted that feature in ssa printing
too.

[gri, 2014-03-28 18:02:20]

PTAL

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go
File cmd/godex/print.go (right):

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode65
cmd/godex/print.go:65: if obj := scope.Lookup(name); obj.Exported() {
On 2014/03/28 15:53:59, adonovan wrote:
> The least restrictive filter is now: Exported().
> I think that makes sense since the maintainer of a package can look at the
> source to find unexported names, but I just wanted to confirm the intent.

for now that's the intent

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode65
cmd/godex/print.go:65: if obj := scope.Lookup(name); obj.Exported() {
On 2014/03/28 15:53:59, adonovan wrote:
> I would move the obj:= assignment out of the if-stmt since it's used in both
> branches, and there's nothing following the if-stmt.

Done.

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode74
cmd/godex/print.go:74: if named, _ := obj.Type().(*types.Named); named != nil &&
named.NumMethods() > 0 {
On 2014/03/28 15:53:59, adonovan wrote:
> named!=nil is a tautology here: how can a TypeName's type be anything other
than
> a Named?

it can be a *Basic :-)

(hence my pending CL, for after 1.3, that eliminates that possibility)

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode89
cmd/godex/print.go:89: // no filtering: collect top-level unexported types with
methods
On 2014/03/28 15:53:59, adonovan wrote:
> Why not apply filtering to unexported names too?

Not for now - the effect is undesirable. For instance, somebody looking for
types.Object would want to see the methods attached to the unexported
types.object - which they wouldn't with filtering.

What exactly filtering should do and what not is a bit of a fine-tuning thing.
Let's not get bogged down here for now. As is works reasonably well, but it
should be improved over time.

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode92
cmd/godex/print.go:92: if named, _ := obj.Type().(*types.Named); named != nil &&
named.NumMethods() > 0 {
On 2014/03/28 15:53:59, adonovan wrote:
> ditto (named!=nil)

see above

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode189
cmd/godex/print.go:189: // TODO(gri) use floating-point notation for exact
floating-point numbers (fractions)
On 2014/03/28 15:53:59, adonovan wrote:
> Perhaps exact.Value should do that?  I've wanted that feature in ssa printing
> too.

we need both at some point

[gri, 2014-03-28 19:22:03]

*** Submitted as
https://code.google.com/p/go/source/detail?r=59ce91dfc20e&repo=tools ***

go.tools/cmd/godex: fix prefix generation, filtering, formatting

Details:
- auto-generate prefixes for std lib (e.g., "godex big" works now)
- apply filtering to package-level objects only
- nicer formatting of single-entry const, var, or type declaration

TBR=adonovan
R=adonovan
CC=golang-codereviews
https://codereview.appspot.com/81360046

[adonovan, 2014-03-28 19:26:44]

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go
File cmd/godex/print.go (right):

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode74
cmd/godex/print.go:74: if named, _ := obj.Type().(*types.Named); named != nil &&
named.NumMethods() > 0 {
On 2014/03/28 18:02:20, gri wrote:
> On 2014/03/28 15:53:59, adonovan wrote:
> > named!=nil is a tautology here: how can a TypeName's type be anything other
> than
> > a Named?
> 
> it can be a *Basic :-)
> 
> (hence my pending CL, for after 1.3, that eliminates that possibility)

But we're inspecting a package scope here, and TypeName objects whose type is a
Basic only live in the universal scope.

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode89
cmd/godex/print.go:89: // no filtering: collect top-level unexported types with
methods
On 2014/03/28 18:02:20, gri wrote:
> On 2014/03/28 15:53:59, adonovan wrote:
> > Why not apply filtering to unexported names too?
> 
> Not for now - the effect is undesirable. For instance, somebody looking for
> types.Object would want to see the methods attached to the unexported
> types.object - which they wouldn't with filtering.
> 
> What exactly filtering should do and what not is a bit of a fine-tuning thing.
> Let's not get bogged down here for now. As is works reasonably well, but it
> should be improved over time.

Fine.

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode92
cmd/godex/print.go:92: if named, _ := obj.Type().(*types.Named); named != nil &&
named.NumMethods() > 0 {
On 2014/03/28 18:02:20, gri wrote:
> On 2014/03/28 15:53:59, adonovan wrote:
> > ditto (named!=nil)
> 
> see above

ditto

[adonovan, 2014-03-28 19:26:52]

LGTM

[gri, 2014-03-28 19:28:38]

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go
File cmd/godex/print.go (right):

https://codereview.appspot.com/81360046/diff/60001/cmd/godex/print.go#newcode74
cmd/godex/print.go:74: if named, _ := obj.Type().(*types.Named); named != nil &&
named.NumMethods() > 0 {
On 2014/03/28 19:26:44, adonovan wrote:
> On 2014/03/28 18:02:20, gri wrote:
> > On 2014/03/28 15:53:59, adonovan wrote:
> > > named!=nil is a tautology here: how can a TypeName's type be anything
other
> > than
> > > a Named?
> > 
> > it can be a *Basic :-)
> > 
> > (hence my pending CL, for after 1.3, that eliminates that possibility)
> 
> But we're inspecting a package scope here, and TypeName objects whose type is
a
> Basic only live in the universal scope.

Not correct: unsafe.Pointer is a basic type that lives in the package scope of
package unsafe.