@pboyer I wasn't sure if I should have forked your tree, or the main tree, as it's a Go-specific change. I'd love to hear your thoughts on this pull request.

Hey @cyberfox,

Thanks for the code.

Couple of comments.
Forking the antlr repo is correct, not pboyer's.

The VisitChildren code isn't idiomatic Go, and I found it quite confusing.
Bellow is an attempt to improve it.

• Testing the type assertions has been replaced with type switches.
• The super != nil code is together to make it easier to read.
• Changed DefaultResult to InitResult, as it is a better description of what is happening.
• The ShouldVisitNextChild methods name is a bit verbose.

func (v *BaseTQueryWalkerVisitor) VisitChildren(node antlr.RuleNode) interface{} {

	if v.super != nil {
		result := v.super.InitResult()
		for _, child := range node.GetChildren() {
			doNextChild := v.super.ShouldVisitNextChild(node, result)
			if !doNextChild {
				break
			}
			switch child := child.(type) {
			case antlr.TerminalNode:
				childResult := v.VisitTerminal(child)
				result = v.super.AggregateResult(result, childResult)
			case antlr.ErrorNode:
				v.VisitErrorNode(child)
			case antlr.RuleNode:
				childResult := child.Accept(v.super)
				result = v.super.AggregateResult(result, childResult)
			default:
				// can this happen??
			}
		}
		return result
	}

	for _, child := range node.GetChildren() {
		switch child := child.(type) {
		case antlr.TerminalNode:
			v.VisitTerminal(child)
		case antlr.ErrorNode:
			v.VisitErrorNode(child)
		case antlr.RuleNode:
			child.Accept(v)
		default:
			// can this happen??
		}
	}
	return nil
}

Thinking about it, this still isn't idiomatic Go.
The super can go, the three signatures (InitResult, ShouldVisitNextChild, AggregateResult) can go into a new interface (Result would be a good name). Instead of testing for super != nil the code would just check if v implements Result.

type Result interface {
  Init() interface{}
  VisitNext(node antlr.RuleNode, resultSoFar interface{}) bool
  Aggregate( resultSoFar interface{}, childResult interface{}) interface{}
}

This does mean the Result would have to be the visitor implementation, but I don't thing this is an issue.

Lastly, a small change to the example makes it clear how use visitor to capture precedence.

func (v *PianoVisitor) VisitSharps(ctx *parser.SharpsContext) interface{} {
	fmt.Printf("In here: %s\n", ctx.GetText())
        // before children
	result := v.VisitChildren(ctx)
        // after children
        return result
}

@cyberfox

It turns out that the visitor only actually works if the delegate (super) is set.

Go is not object-oriented in the same way as Java, and "super" classes don't delegate to "subclasses".

I've pushed code to
gmwxio@61cf63d

I want to look at another way of doing this before sending you a PR.

@cyberfox take a look at https://github.com/wxio/antlr4/tree/go-visitor

If there are any error its because I'm working a branch with other features and extracted the visitor changes, but might have made a mistake.

This is now more Go like.
There is an interface for each visit method, and the callback is handled to the VisitChildren method instead of having a "super" field.
It's now cleaner and more flexible.

Cheers
Gary

@millergarym I'd love to take a look at your version! It would be helpful if you could show how someone would implement the visitor pattern using your code? I think I'm getting what you're trying to do, but an example (like the one I posted at the top of the pull request) would make it much easier to follow. Also, instead of commenting out lines, go ahead and delete them. It's version control, I have them still. :)

For what it's worth, I specifically pointed out that it requires a little extra work (that being the SetSuper(...) call above) for a visitor client. I wasn't relying on super existing. I actually have a mildly complex project that's been built on top of the existing code; I'm happy to swap the parser part out if we find a more Go-appropriate style, but I promise the code I put in is working. :)

I am new to Go, so I expect my code isn't 'Go like', though.

is there any update on this issue?

Is it still unresolved? The original issue writer might not be as interested in the issue but we still a fix

Any status update?

I ended up having to do other things for a while, and having to put my rules engine (my itch that I'm scratching with this) on hold. I'm open to making any changes needed to help with this, I got the idea that I'm misunderstanding how this is supposed to be used.

I also was definitely wrong to do this from master, so I need to split off a branch...

I would be quite interested in this feature. Is there anyone who is currently working on it? Maybe @cyberfox?