code review 166047: the AST walker currently provides no way to find out how the

the AST walker currently provides no way to find out how the
nodes in the tree are nested with respect to one another.
a simple change to the Visitor interface makes it possible
to do this (for example to maintain a current node-depth, or a
knowledge of the name of the current function).

Visit(nil) is called at the end of a node's children;
this make possible the channel-based interface below,
amongst other possibilities.

It is still just as simple to get the original behaviour - just
return the same Visitor from Visit.

Here are a couple of possible Visitor types.

// closure-based
type FVisitor func(n interface{}) FVisitor
func (f FVisitor) Visit(n interface{}) Visitor {
	return f(n);
}

// channel-based
type CVisitor chan Visit;
type Visit struct {
	node interface{};
	reply chan CVisitor;
};
func (v CVisitor) Visit(n interface{}) Visitor
{
	if n == nil {
		close(v);
	} else {
		reply := make(chan CVisitor);
		v <- Visit{n, reply};
		r := <-reply;
		if r == nil {
			return nil;
		}
		return r;
	}
	return nil;
}

[rog, 2009-12-03 23:23:01]

Hello r, rsc (cc: rog),

I'd like you to review the following change.

[rog, 2009-12-03 23:27:17]

one other thing: i changed Walk so that it's possible to call it directly on the
array fields inside the node, so that all clients need not reimplement
walkFieldList etc.
YMMV.

[gri, 2009-12-04 22:25:56]

Good idea. Some comments below.

http://codereview.appspot.com/166047/diff/1012/1013
File src/cmd/godoc/index.go (right):

http://codereview.appspot.com/166047/diff/1012/1013#newcode512
src/cmd/godoc/index.go:512: if node == nil {
remove in favor or case nil: below

http://codereview.appspot.com/166047/diff/1012/1013#newcode516
src/cmd/godoc/index.go:516: switch n := node.(type) {
case nil:
   break;

http://codereview.appspot.com/166047/diff/1012/1014
File src/pkg/go/ast/walk.go (right):

http://codereview.appspot.com/166047/diff/1012/1014#newcode11
src/pkg/go/ast/walk.go:11: // If Visit returns non-nil, it's a Visitor that will
be used traverse the node's
change 2nd sentence to:

// If the result visitor w is not nil, Walk visits each of the children
// of node with the visitor w, followed by a call of w.Visit(nil).

http://codereview.appspot.com/166047/diff/1012/1014#newcode14
src/pkg/go/ast/walk.go:14: Visit(node interface{}) Visitor;
s/Visitor/(w Visitor)/

http://codereview.appspot.com/166047/diff/1012/1014#newcode39
src/pkg/go/ast/walk.go:39: // Walk traverses an AST in depth-first order and
invokes v.Visit(n) for each
Change to:

// Walk traverses an AST in depth-first order: If node != nil, it invokes
v.Visit(node).
// If the visitor w returned by v.Visit(node) is not nil, Walk visits each of
the children
// of node with the visitor w, followed by a call of w.Visit(nil).

http://codereview.appspot.com/166047/diff/1012/1014#newcode53
src/pkg/go/ast/walk.go:53: // of the corresponding declaration in go)
The declarations of these types are in ast.go, not go. Please revert.

http://codereview.appspot.com/166047/diff/1012/1014#newcode298
src/pkg/go/ast/walk.go:298: }
please match the existing style and add an empty line between the cases below

[rog, 2009-12-07 10:48:17]

done (with one additional comment indicating that arrays are allowed in Walk).

[gri, 2009-12-07 18:17:35]

LGTM.

Thanks. I'll fix the typos.

http://codereview.appspot.com/166047/diff/2007/1021
File src/pkg/go/ast/walk.go (right):

http://codereview.appspot.com/166047/diff/2007/1021#newcode45
src/pkg/go/ast/walk.go:45: // on of the nodes in the array.
minor typos here

[gri, 2009-12-07 18:33:49]

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

the AST walker currently provides no way to find out how the
nodes in the tree are nested with respect to one another.
a simple change to the Visitor interface makes it possible
to do this (for example to maintain a current node-depth, or a
knowledge of the name of the current function).

Visit(nil) is called at the end of a node's children;
this make possible the channel-based interface below,
amongst other possibilities.

It is still just as simple to get the original behaviour - just
return the same Visitor from Visit.

Here are a couple of possible Visitor types.

// closure-based
type FVisitor func(n interface{}) FVisitor
func (f FVisitor) Visit(n interface{}) Visitor {
	return f(n);
}

// channel-based
type CVisitor chan Visit;
type Visit struct {
	node interface{};
	reply chan CVisitor;
};
func (v CVisitor) Visit(n interface{}) Visitor
{
	if n == nil {
		close(v);
	} else {
		reply := make(chan CVisitor);
		v <- Visit{n, reply};
		r := <-reply;
		if r == nil {
			return nil;
		}
		return r;
	}
	return nil;
}

R=gri
CC=rsc
http://codereview.appspot.com/166047

Committer: Robert Griesemer <gri@golang.org>

[rog, 2009-12-07 20:01:37]

On 2009/12/07 18:33:49, gri wrote:
> // closure-based
> type FVisitor func(n interface{}) FVisitor
> func (f FVisitor) Visit(n interface{}) Visitor {
> 	return f(n);
> }

just noticed, this isn't submitted code (although
it came from some of my code), but this has the standard
typed-nil-becomes-non-nil-interface bug.

it should be something like:

func (f FVisitor) Visit(n interface{}) Visitor {
	if f := f(n); f != nil {
		return f;
	}
	return nil;
}