code review 164460044: cmd/callgraph: a utility for dumping the callgraph of a...

cmd/callgraph: a utility for dumping the callgraph of a Go program.

(This functionality is provided by the oracle, but its output
format is inflexible, and the functionality is better suited
to a shell utility.  I may remove the oracle 'callgraph' feature.)

See Usage for details.

+ Test.

[adonovan, 2014-11-06 20:04:45]

Hello sameer@golang.org (cc: golang-codereviews@googlegroups.com,
gri@golang.org),

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

[Sameer Ajmani, 2014-11-07 14:41:26]

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go
File cmd/callgraph/main.go (right):

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:41: `Call graph construction algorithm, one of "rta" or
"pta"`)
define these tlas

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:43: var testFlag = flag.Bool("test", false,
include_tests

--test sounds like you're testing the tool

Per our chat, it sounds like this does something else; it looks at test mains
instead of program mains.

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:54: callgraph [-algo=rta|pta] [-test] [-format=...]
<args>...
what are <args>... ?

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:76: Caller string
what is the form of caller and callee?

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:80: Kind   string
what values can kind take?

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:90: # Show all dynamic calls originating from the tests of
the 'fmt' package:
Start with a simpler example, please.  One with no pipeline.

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:220: // TODO(adonovan): opt: visit nodes once and put all
successors on one line?
You'd want to change .Callee to a slice of .Callees.  That's an incompatible
change to your tool API, so you'd best do it in this CL or remove this TODO.  I
think it would be better to do it now.

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:226: format = `  {{printf "%q" .Caller}} -> {{printf "%q"
.Callee}}"`
Don't you need a newline or semicolon after this?

With the Callees change, this becomes a range:
format = `{{range callee := .Callees}}  {{printf "%q" .Caller}} -> {{printf "%q"
$callee}};
{{end}}`

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main_test.go
File cmd/callgraph/main_test.go (right):

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main_test.g...
cmd/callgraph/main_test.go:17: const format = "{{.Caller}} --> {{.Callee}}"
Test digraph and graphviz formats.

[adonovan, 2014-11-07 20:10:36]

PTAL

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go
File cmd/callgraph/main.go (right):

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:41: `Call graph construction algorithm, one of "rta" or
"pta"`)
On 2014/11/07 14:41:26, Sameer Ajmani wrote:
> define these tlas

They're explained in the -help message.

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:43: var testFlag = flag.Bool("test", false,
On 2014/11/07 14:41:25, Sameer Ajmani wrote:
> include_tests
> 
> --test sounds like you're testing the tool
> 
> Per our chat, it sounds like this does something else; it looks at test mains
> instead of program mains.

We use this flag in all the go/loader-based command-line tools.
The flag docstring explanation seems clear enough.

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:43: var testFlag = flag.Bool("test", false,
On 2014/11/07 14:41:25, Sameer Ajmani wrote:
> include_tests
> 
> --test sounds like you're testing the tool
> 
> Per our chat, it sounds like this does something else; it looks at test mains
> instead of program mains.

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:54: callgraph [-algo=rta|pta] [-test] [-format=...]
<args>...
On 2014/11/07 14:41:25, Sameer Ajmani wrote:
> what are <args>... ?

The constant loader.FromArgsUsage explains it.
It's hard to see in the code but it looks good when actually run.

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:76: Caller string
On 2014/11/07 14:41:26, Sameer Ajmani wrote:
> what is the form of caller and callee?

Documented.

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:80: Kind   string
On 2014/11/07 14:41:26, Sameer Ajmani wrote:
> what values can kind take?

Documented.

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:90: # Show all dynamic calls originating from the tests of
the 'fmt' package:
On 2014/11/07 14:41:26, Sameer Ajmani wrote:
> Start with a simpler example, please.  One with no pipeline.

Done---though this tool will always be used in a pipeline.

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:220: // TODO(adonovan): opt: visit nodes once and put all
successors on one line?
On 2014/11/07 14:41:26, Sameer Ajmani wrote:
> You'd want to change .Callee to a slice of .Callees.  That's an incompatible
> change to your tool API, so you'd best do it in this CL or remove this TODO. 
I
> think it would be better to do it now.

Yes, it could be an incompatible change to the CLI, although one possibility is
to just move the output logic below into this case; it doesn't have to be
expressable as a template.  (Notice that "graphviz" already pulls tricks like
this to add a prefix and suffix.)

More generally: I don't know what the right answer is for design questions like
this.  Expose maximum generality and you make the common case hard.  I'm tempted
to keep it simple.  'go list', a tool which also uses text/template, is
instructive: it provides a -json flag which bypasses the template-based
formatting and uses different logic.  We can always add flags later.

https://codereview.appspot.com/164460044/diff/10002/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:226: format = `  {{printf "%q" .Caller}} -> {{printf "%q"
.Callee}}"`
On 2014/11/07 14:41:26, Sameer Ajmani wrote:
> Don't you need a newline or semicolon after this?
> 
> With the Callees change, this becomes a range:
> format = `{{range callee := .Callees}}  {{printf "%q" .Caller}} -> {{printf
"%q"
> $callee}};
> {{end}}`

The printing logic below adds a newline if the template didn't do so.

[Sameer Ajmani, 2014-11-12 17:51:09]

LGTM

Please address my comments before submitting.

https://codereview.appspot.com/164460044/diff/70001/cmd/callgraph/main.go
File cmd/callgraph/main.go (right):

https://codereview.appspot.com/164460044/diff/70001/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:255: // Allocate these once, outside the loop.
s/loop/traversal/

https://codereview.appspot.com/164460044/diff/70001/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:282: type Edge struct {
unexport?

https://codereview.appspot.com/164460044/diff/70001/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:287: Caller *ssa.Function
move the exported fields group above the unexported ones,
so it more closely mirrors the Usage comment.

https://codereview.appspot.com/164460044/diff/70001/cmd/callgraph/main_test.go
File cmd/callgraph/main_test.go (right):

https://codereview.appspot.com/164460044/diff/70001/cmd/callgraph/main_test.g...
cmd/callgraph/main_test.go:17: const format = "{{.Caller}} --> {{.Callee}}"
Test the predefined formats: digraph and graphviz.

[adonovan, 2014-11-12 18:24:32]

https://codereview.appspot.com/164460044/diff/70001/cmd/callgraph/main.go
File cmd/callgraph/main.go (right):

https://codereview.appspot.com/164460044/diff/70001/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:255: // Allocate these once, outside the loop.
On 2014/11/12 17:51:09, Sameer Ajmani wrote:
> s/loop/traversal/

Done.

https://codereview.appspot.com/164460044/diff/70001/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:282: type Edge struct {
On 2014/11/12 17:51:09, Sameer Ajmani wrote:
> unexport?

Lowercase 'edge' would make the logic harder to read.  Also, Edge is part of the
interface of this package, and is documented under the capitalized name, so the
current name makes sense morally as well as practically.

https://codereview.appspot.com/164460044/diff/70001/cmd/callgraph/main.go#new...
cmd/callgraph/main.go:287: Caller *ssa.Function
On 2014/11/12 17:51:09, Sameer Ajmani wrote:
> move the exported fields group above the unexported ones,
> so it more closely mirrors the Usage comment.

Done.

[adonovan, 2014-11-12 22:36:32]

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

cmd/callgraph: a utility for dumping the callgraph of a Go program.

(This functionality is provided by the oracle, but its output
format is inflexible, and the functionality is better suited
to a shell utility.  I may remove the oracle 'callgraph' feature.)

See Usage for details.

+ Test.

LGTM=sameer
R=sameer
CC=golang-codereviews, gri
https://codereview.appspot.com/164460044