analyzer_experimental snapshot

analyzer_experimental snapshot

R=brianwilkerson@google.com, messick@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=23515

[Brian Wilkerson, 2013-06-01 19:46:58]

LGTM, pending Steve's review

https://codereview.chromium.org/16302002/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/visitor/BreadthFirstVisitor.java
(right):

https://codereview.chromium.org/16302002/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/visitor/BreadthFirstVisitor.java:29:
private final LinkedList<ASTNode> queue = new LinkedList<ASTNode>();
createQueue might have been defined in order to allow subclasses to override it.
I've added Steve as a reviewer.

[messick, 2013-06-01 21:32:10]

I didn't look at anything other than BreadthFirstVisitor, and I question the
need for those changes. I could be convinced with some data, though.

https://codereview.chromium.org/16302002/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/visitor/BreadthFirstVisitor.java
(right):

https://codereview.chromium.org/16302002/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/visitor/BreadthFirstVisitor.java:29:
private final LinkedList<ASTNode> queue = new LinkedList<ASTNode>();
On 2013/06/01 19:46:58, Brian Wilkerson wrote:
> createQueue might have been defined in order to allow subclasses to override
it.
> I've added Steve as a reviewer.

Just so. Why delete it?

https://codereview.chromium.org/16302002/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/visitor/BreadthFirstVisitor.java:47:
ASTNode next = queue.removeFirst();
None of these changes are needed, unless this is showing up in performance
profiling.

[scheglov, 2013-06-01 22:12:03]

https://codereview.chromium.org/16302002/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/visitor/BreadthFirstVisitor.java
(right):

https://codereview.chromium.org/16302002/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/visitor/BreadthFirstVisitor.java:47:
ASTNode next = queue.removeFirst();
On 2013/06/01 21:32:10, messick wrote:
> None of these changes are needed, unless this is showing up in performance
> profiling.

In Dart there are no direct replacement for LinkedList.
The closest analog is Queue, but it is not the List.

So, there are two options:
1. implement JavaLinkedList;
2. change code to use Queue compatible API.

I've checked that there are no overrides for createQueue().
And actually we don't need List here, we need a queue.
So, I've thought that it is better to change this class.

[messick, 2013-06-01 22:30:24]

On 2013/06/01 22:12:03, scheglov wrote:
>
https://codereview.chromium.org/16302002/diff/1/editor/tools/plugins/com.goog...
> File
>
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/visitor/BreadthFirstVisitor.java
> (right):
> 
>
https://codereview.chromium.org/16302002/diff/1/editor/tools/plugins/com.goog...
>
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/visitor/BreadthFirstVisitor.java:47:
> ASTNode next = queue.removeFirst();
> On 2013/06/01 21:32:10, messick wrote:
> > None of these changes are needed, unless this is showing up in performance
> > profiling.
> 
> In Dart there are no direct replacement for LinkedList.
> The closest analog is Queue, but it is not the List.
> 
> So, there are two options:
> 1. implement JavaLinkedList;
> 2. change code to use Queue compatible API.
> 
> I've checked that there are no overrides for createQueue().
> And actually we don't need List here, we need a queue.
> So, I've thought that it is better to change this class.

I see. I wasn't expecting that class to be translated to Dart.
It was intended for UI code in the editor. But, it could be
translated, I suppose.

I don't have a strong opinion.

LGTM (with your changes)

[scheglov, 2013-06-02 22:00:48]

Committed patchset #1 manually as r23515 (presubmit successful).