Add an iterative version of the ParseTreeWalker #1231
Conversation
|
|
||
| @Override | ||
| public void walk(final ParseTreeListener listener, final ParseTree t) { | ||
| for (WalkerNode<?> currentNode = createNode(null, t, 0); currentNode != null;) { |
There was a problem hiding this comment.
💡 This implementation requires at least one allocated object for every node in the tree. You could avoid all these allocations by instead using two stacks:
Deque<SyntaxTree> treeStack = new ArrayDeque<SyntaxTree>();
IntegerStack currentIndex = new IntegerStack();Where the node you are currently visiting is treeStack.peek() and its index in the parent is currentIndex.peek().
There was a problem hiding this comment.
You're right. I thought if I'd use a LinkedList for the stack, then I'd have object allocation anyways, just hidden inside the LinkedList. But I see that ArrayDeque does a better job here.
| @@ -0,0 +1,151 @@ | |||
| /* | |||
| * [The "BSD license"] | |||
| * Copyright (c) 2012 Terence Parr | |||
There was a problem hiding this comment.
Isn't that copyright wrong? Neither exists this file since 2012 nor was it written by Sam + Ter.
There was a problem hiding this comment.
The year is particularly interesting. It turns out keeping the year constant and using the year when the code base started provides the longest and strongest copyright protections. If you want to see some of the details on this, take a look at the long and probably very boring comments in DotNetAnalyzers/StyleCopAnalyzers#1661.
The use of mine and Terence's names has to do with copyright assignment, which is (to the best of my knowledge) common practice in a number of circles. There are three ways we work to ensure the original author of patches remains recognized for the life of the project:
- The contributors.txt file, which is an essential and core part of this project. It's not distributed, but the distribution links back to it.
- The Git commit history.
- The pull requests and issues filed by everyone.
📝 I am not a lawyer
There was a problem hiding this comment.
Hmm, you certainly have your reasons, but to me it feels wrong. The header says the file exists since 2012, which is plain wrong. If you always use the same year then you wouldn't need individual copyrights in each file, but just can have a central COPYRIGHT file and save the work to maintain a per-file copyright. Anway, even though I don't agree with your reasoning, I certainly don't want to open a big discussion about this here. Thanks for explaining.
There was a problem hiding this comment.
I'm fine with everything here. Just tell me what, in your opinion, should be the copyright header, or if I can remove that completely? I must admit that I just copy/pasted the copyright notice from another file, trying to adhere to the code standards of this project, and not paying much attention to the header's content.
There was a problem hiding this comment.
note I'm going to blast all headers to have consistent BSD license. I feel that the git history is best way to determine authorship. Adding a name to the license comment is very misleading/ambiguous sometimes. Did that person fix a typo or write most of it?
There was a problem hiding this comment.
Shall I wipe out the header from this file, then?
|
@twz123 we need a sig in the contributors.txt file :) |
|
@ericvergnaud What is best way to "broadcast" to target authors that a new bit of functionality has been added to one target? |
|
Have them subscribe to all changes (which is what I do) |
|
@parrt Isn't the ANTLR mailinglist supposed to serve as a central communication means between ANTLR developers? Even though there are many user questions the main purpose should be the discussions/exchange about the development of ANTLR, IMO. So, you could send out a notification mail about the addition and have target authors reply with their time estimates for implementation (or, if there is no reply for particular target you know nobody is taking care) and also report the completion. Alternatively, you could add a comment with all the known target authors in Github when you merge a new feature, so they will all get a mail about this. Better than to subscribe to all changes. |
|
@parrt I believe it would be very helpful if each change/addition of runtime code must also name an existing testcase (or a new one, if there is none yet) in the PR, which tests the changed/added code. Target authors then can use the same test for their implementations. |
|
@ericvergnaud @mike-lischke how about I create a team with target folks and then ref the team in comments that they should notice? |
|
@twz123 we need a sig in the contributors.txt file :) |
|
@parrt Yes, a Github team could work as well. |
|
let’s try that and see how it goes
|
|
@parrt Added myself to contributors at the date of my first commit. If I should better do it using the current date, just tell me. Would you like me to rebase this thing? |
|
Note to @antlr/antlr-targets that a new tree walker is enter the runtime, if you would like to implement it. |
I hit a
StackOverflowErrorwhile traversing deeply nested trees using the standardParseTreeWalker, so I decided to implement an iterative version of it using a linked data structure instead of recursion during traversal.