Added code to associate a dart doc comment with a dart declaration node.

Added code to associate a dart doc comment with a dart declaration node.
Committed: https://code.google.com/p/dart/source/detail?r=2090

[devoncarew, 2011-12-02 18:06:14]

Miguel/John: we're looking to plumb dartdoc support through from the compiler to
the UI. We'd like to have some dartdoc hover support hopefully for the hackathon
next week. This CL does the work to associate dartdoc comments with
DartDeclaration nodes. Brian will then pick up where this CL leaves off and move
the information from the compiler's AST to the editor's AST.

[Brian Wilkerson, 2011-12-02 18:25:33]

LGTM

[codefu, 2011-12-02 18:27:39]

On 2011/12/02 18:06:14, devoncarew wrote:
> Miguel/John: we're looking to plumb dartdoc support through from the compiler
to
> the UI. We'd like to have some dartdoc hover support hopefully for the
hackathon
> next week. This CL does the work to associate dartdoc comments with
> DartDeclaration nodes. Brian will then pick up where this CL leaves off and
move
> the information from the compiler's AST to the editor's AST.

Curious why you didn't overload some of the parser methods handling declarations
to link the docComments to the declarations.  Time constraint?

[Brian Wilkerson, 2011-12-02 19:06:12]

> Curious why you didn't overload some of the parser methods handling
declarations
> to link the docComments to the declarations.  Time constraint?

I was going to respond with a description of how hard it would be in that
implementation to ensure that the corner cases in which comments of the form
"/** ... */" that were not immediately before the first token of the declaration
didn't get erroneously associated with the wrong declaration when I realized
that the approach in this CL doesn't properly guard against that case either.
Consider the following example:

  void firstMethod() {
    ...
    /** this should be ignored */
  }

  void secondMethod() {
    ...
  }

I believe that the comment inside the first method should *not* be associated
with the second method, but that this CL would have that effect. The question
then becomes: how important is it to handle these corner cases? I think it is
actually very important because it would be confusing to users to have us get it
wrong and shouldn't be that hard. I rescind my LG.

[devoncarew, 2011-12-02 19:07:21]

> Curious why you didn't overload some of the parser methods handling
declarations
> to link the docComments to the declarations.  Time constraint?

I would say mostly unfamiliarity with the source base. If you think there's a
better way to capture the doc comments I'm all for it. I was mostly taking the
existing code to work with doc comments and extending it for our purposes.

You're suggesting overloading or modifying methods like parseClass() to take the
last doc comment and apply it to the current Node? You'd then have to clear out
the last doc comment for every node that wasn't a declaration node.

[devoncarew, 2011-12-02 19:07:50]

> wrong and shouldn't be that hard. I rescind my LG.

Nooo!

[codefu, 2011-12-02 19:22:36]

On 2011/12/02 19:06:12, Brian Wilkerson wrote:
> > Curious why you didn't overload some of the parser methods handling
> declarations
> > to link the docComments to the declarations.  Time constraint?
> 
> I was going to respond with a description of how hard it would be in that
> implementation to ensure that the corner cases in which comments of the form
> "/** ... */" that were not immediately before the first token of the
declaration
> didn't get erroneously associated with the wrong declaration when I realized
> that the approach in this CL doesn't properly guard against that case either.
> Consider the following example:
> 
>   void firstMethod() {
>     ...
>     /** this should be ignored */
>   }
> 
>   void secondMethod() {
>     ...
>   }
> 
> I believe that the comment inside the first method should *not* be associated
> with the second method, but that this CL would have that effect. The question
> then becomes: how important is it to handle these corner cases? I think it is
> actually very important because it would be confusing to users to have us get
it
> wrong and shouldn't be that hard. I rescind my LG.

I was going to write that "secondMethod and DartComment(this hsould be ignored)
do not share the same parent", then I double checked and noticed comments are
generated _after_ the parsing and not in the AST during parse... so that's out.

[Brian Wilkerson, 2011-12-02 19:30:36]

> ... then I double checked and noticed comments are
> generated _after_ the parsing and not in the AST during parse...

Yes. I think there is a strong case to be made for rolling
CommentPreservingParser up into DartParser. Most of the comments still need to
be associated with the compilation unit, but DartDoc comments that can be
associated with a declaration really ought to have the declaration be their
parent. But that's more work than we can do today.

[devoncarew, 2011-12-05 09:17:28]

PTAL, I added a guard against dartdocs contained in sibling nodes, and added a
test to ensure that we are assigning dartdocs to declaration nodes.

[Brian Wilkerson, 2011-12-05 16:14:24]

LGTM

http://codereview.chromium.org/8769044/diff/9005/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/parser/CommentPreservingParser.java
(right):

http://codereview.chromium.org/8769044/diff/9005/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/parser/CommentPreservingParser.java:154:
public DartNode visitNode(DartNode node) {
We could improve the performance by overriding visitDeclaration(DartDeclaration)
rather than visitNode(DartNode).

[mmendez, 2011-12-05 17:58:49]

lgtm

http://codereview.chromium.org/8769044/diff/9005/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/parser/CommentPreservingParser.java
(right):

http://codereview.chromium.org/8769044/diff/9005/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/parser/CommentPreservingParser.java:208:
private List<DartNode> getChildren(DartNode parent) {
Nit: You are probably redoing a lot of work here when dealing with methods
declared on a class or when there are a lot of top level classes.  Perhaps a
range sort might be better.

[devoncarew, 2011-12-05 19:30:38]

http://codereview.chromium.org/8769044/diff/9005/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/parser/CommentPreservingParser.java
(right):

http://codereview.chromium.org/8769044/diff/9005/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/parser/CommentPreservingParser.java:154:
public DartNode visitNode(DartNode node) {
On 2011/12/05 16:14:24, Brian Wilkerson wrote:
> We could improve the performance by overriding
visitDeclaration(DartDeclaration)
> rather than visitNode(DartNode).

Done.

http://codereview.chromium.org/8769044/diff/9005/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/parser/CommentPreservingParser.java:208:
private List<DartNode> getChildren(DartNode parent) {
On 2011/12/05 17:58:50, mmendez wrote:
> Nit: You are probably redoing a lot of work here when dealing with methods
> declared on a class or when there are a lot of top level classes.  Perhaps a
> range sort might be better.

Good catch - it looks like we are unnecessarily re-doing calculations. From
thinking about it, I think I prefer the simplicity of this method to the added
complexity of caching the child information for the nodes. It's fairly
light-weight to re-calculate. Unless I hear objections, I'll submit this CL as
is -