Dartdoc comments retrieved as metadata through dart2js mirrors.

Dartdoc comments retrieved as metadata through dart2js mirrors.

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

[Andrei Mouravski, 2013-01-02 16:56:06]

Looks good, but I'd love to see some tests so it's unambiguous how comments
work. Some test cases I'd like to see:

/// Doc comment 1
// Plain comment
/**
 * Doc comment 2
 */
class foo { }

/* Comment */ class bar

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:2165: * The
beginning token of this annotation, or [:null:] if it is synthetic.
Quick question: what is meant by "synthetic"? It's not defined anywhere that I
can see, nor is it defined in the spec.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart
(right):

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1796:
String _stripComment(String comment) {
Why not make this public or even put it into a utils library somewhere? It seems
like something that can be useful elsewhere.

[ahe, 2013-01-03 13:04:09]

+1 for more tests of strange comments. Ideally, this would be tested by
standalone tests which suggest that the _stripComment function should be public
in its own (implementation) library.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:106: class
CommentTokenMapEntry {
Document class please. How about:

/// An entry in [CommentTokenMap].

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:107: final key;
Add types one these two fields please.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:114: * Map of tokens and
the first associated comment.
One line comment?

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:120: for
(CommentTokenMapEntry entry in entries) {
This seems like a sub-optimal implementation. I would like to understand why
this is doesn't cause quadratic runtime before approving this change.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:2165: * The
beginning token of this annotation, or [:null:] if it is synthetic.
On 2013/01/02 16:56:06, Andrei Mouravski wrote:
> Quick question: what is meant by "synthetic"? It's not defined anywhere that I
> can see, nor is it defined in the spec.

This is compiler lingo (at least where I'm from). It means something that isn't
real (as in present in source code) and that the compiler has made up.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart
(right):

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:549:
_appendCommentTokens(mirrors.compiler.commentMap[metadata.beginToken]);
I think this has horrible performance.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:554:
_appendCommentTokens(mirrors.compiler.commentMap[getBeginToken()]);
Ditto.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1799: //
Allow a leading space.
allow -> strip.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1802: }
else {
I suggest you handle this case differently, per my comments below.

First, remove leading r'/\*\* ?' from comment.
Then, remove trailing r' ?\*/' from comment.
Now split the lines.  Add the first line to buf.
Remove trailing r'[ \t]*\* ?' of the remaining lines (not the first) and add
them to buf. For example:

for (var line in comment.split('\n')) {
  if (buf.isEmpty) {
    buf.add(line); // Add the first line unprocessed.
    continue;
  }
  buf.add('\n');
  Match match = new RegExp(r'^[ \t]*\* ?(.*)').firstMatch(line);
  if (match != null) line = match[1];
  buf.add(line);
}

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1805: line
= line.trim();
This deletes trailing whitespace, unlike how you handle ///.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1806: if
(line.startsWith('/**')) line = line.substring(3);
You don't need to trim the line to test for this.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1807: if
(line.endsWith('*/')) line = line.substring(0, line.length - 2);
You don't need to trim the line to test for this.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1808: line
= line.trim();
I don't understand why you're trimming again.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1809: if
(line.startsWith('* ')) {
This has weird behavior for comments like this:

/** *Important:* Don't remove too much! */

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1832:
mirrors.compiler.libraryLoader.loadLibrary(uri, null, uri);
This seems overly lazy. Can you initialize the object with this information
instead?

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1865: new
StringConstant(new DartString.literal(trimmedText), null)));
You might benefit from a new constructor on Dart2JsStringConstantMirror that
just takes a String.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/dartdoc/lib...
File sdk/lib/_internal/dartdoc/lib/dartdoc.dart (right):

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/dartdoc/lib...
sdk/lib/_internal/dartdoc/lib/dartdoc.dart:1872: 
Extra line.

[ahe, 2013-01-03 13:04:54]

I'm not saying go because I'm concerned with the performance of the token to
comment mapping.

[Johnni Winther, 2013-01-03 13:19:32]

On 2013/01/03 13:04:54, ahe wrote:
> I'm not saying go because I'm concerned with the performance of the token to
> comment mapping.

I actually tested several implementations. Using a Map<Token,Token> was horrible
(4 times the scanning time) and storing the comment token in a field on Token
had the same performance as using the linked list of entries. I don't know why
the linear search in CommentTokenMap.[] does not cause the theoretical worst
case time complexity. The CL gives comparable (and sometimes faster) running
times for apidoc.

[ahe, 2013-01-03 13:31:00]

On 2013/01/03 13:19:32, Johnni Winther wrote:
> I actually tested several implementations. Using a Map<Token,Token> was
horrible
> (4 times the scanning time)

That is to be expected. The VMs implementation of Object.hashCode is horrible.

> and storing the comment token in a field on Token
> had the same performance as using the linked list of entries. I don't know why
> the linear search in CommentTokenMap.[] does not cause the theoretical worst
> case time complexity. The CL gives comparable (and sometimes faster) running
> times for apidoc.

Did you try using Expando? It might be just as horrible as my understanding is
that this is what the VM uses internally for hashCode.

I still don't believe this isn't too slow. How about using a map of TokenKey to
Token:

class TokenKey {
  final Token token;
  TokenKey(this.token);
  int get hashCode => token.charOffset; // Only good hash if you create one map
per file.
  operator==(other) => other is TokenKey && token == other.token;
}

[Andrei Mouravski, 2013-01-03 17:41:46]

After you determine which method is most efficient, could you add a // comment
in the code itself outlining this discussion so that future maintainers can have
the benefit of this code review without having to look it up?

Something as simple as:
// This method is testably more efficient than Map<Token, Token> blah blah
// but if the following foo is changed in the VM then method bar will be more
efficient.

[Johnni Winther, 2013-01-04 16:40:53]

PTAL

New commentMap implementation class TokenKeyMap including reasoning and
measurements.
stripComment is moved to mirrors_util and a test is added.
mirrors_test is extended with test of comments as metadata.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:2165: * The
beginning token of this annotation, or [:null:] if it is synthetic.
On 2013/01/03 13:04:09, ahe wrote:
> On 2013/01/02 16:56:06, Andrei Mouravski wrote:
> > Quick question: what is meant by "synthetic"? It's not defined anywhere that
I
> > can see, nor is it defined in the spec.
> 
> This is compiler lingo (at least where I'm from). It means something that
isn't
> real (as in present in source code) and that the compiler has made up.

Yes. Currently we have a 'patch' pseudo-modifier for which we create an
annotation. This annotation is therefore synthetic.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:2168: 
Implemented in https://codereview.chromium.org/11729004/ where it was needed.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart
(right):

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1832:
mirrors.compiler.libraryLoader.loadLibrary(uri, null, uri);
On 2013/01/03 13:04:09, ahe wrote:
> This seems overly lazy. Can you initialize the object with this information
> instead?

The class element is now loaded in [Compiler.scanBuiltinLibraries] if
[Compiler.preserveComments] is true.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:1865: new
StringConstant(new DartString.literal(trimmedText), null)));
On 2013/01/03 13:04:09, ahe wrote:
> You might benefit from a new constructor on Dart2JsStringConstantMirror that
> just takes a String.

Done.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/patch_parser.dart (right):

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/patch_parser.dart:506: Token get
beginToken => null;
Implemented in https://codereview.chromium.org/11729004/ where it was needed.

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/dartdoc/lib...
File sdk/lib/_internal/dartdoc/lib/dartdoc.dart (right):

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/dartdoc/lib...
sdk/lib/_internal/dartdoc/lib/dartdoc.dart:1872: 
On 2013/01/03 13:04:09, ahe wrote:
> Extra line.

Done.

[Andrei Mouravski, 2013-01-04 17:46:24]

Good stuff!

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:120: * This
implementation was chosen among several candidates for its space/time
Since it is an implementation detail, this rationale could go into a non doc
comment.

/// Map of tokens and the first associated comment.
/* 
 * This implementation...
 * ...
 */

[ahe, 2013-01-08 11:59:42]

LGTM!

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11726005/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:2165: * The
beginning token of this annotation, or [:null:] if it is synthetic.
On 2013/01/04 16:40:53, Johnni Winther wrote:
> On 2013/01/03 13:04:09, ahe wrote:
> > On 2013/01/02 16:56:06, Andrei Mouravski wrote:
> > > Quick question: what is meant by "synthetic"? It's not defined anywhere
that
> I
> > > can see, nor is it defined in the spec.
> > 
> > This is compiler lingo (at least where I'm from). It means something that
> isn't
> > real (as in present in source code) and that the compiler has made up.
> 
> Yes. Currently we have a 'patch' pseudo-modifier for which we create an
> annotation. This annotation is therefore synthetic.

Not a good example of terminology, as the patch keyword is present in the source
code :-)

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:132: class TokenKeyMap {
This really is a TokenMap.

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart
(right):

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:478:
abstract class Dart2JsTypeMirror extends Dart2JsDeclarationMirror
This class defines no behavior, so it has no purpose.

Chances are that you have code somewhere that says:

if (m is Dart2JsTypeMirror) doSomething(x);

Generally, I'd prefer:

m.doSomething();

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:480: 
Extra line.

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:483:
abstract class Dart2JsElementMirror extends Dart2JsDeclarationMirror {
Generally, I prefer having the super class first. Makes the code easier to read.

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart
(right):

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart:137: while
(lines.hasNext) {
lines.moveNext()

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart:148: } else
if (lines.hasNext || !line.trim().isEmpty) {
This wont work with the new iterators :-(

https://codereview.chromium.org/11726005/diff/3002/tests/compiler/dart2js/str...
File tests/compiler/dart2js/strip_comment_test.dart (right):

https://codereview.chromium.org/11726005/diff/3002/tests/compiler/dart2js/str...
tests/compiler/dart2js/strip_comment_test.dart:16: testComment('', '// ');
Did you mean:
 testComment(' ', '//  ');

https://codereview.chromium.org/11726005/diff/3002/tests/compiler/dart2js/str...
tests/compiler/dart2js/strip_comment_test.dart:24: testComment('', '/// ');
Ditto?

[Johnni Winther, 2013-01-08 13:39:46]

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:120: * This
implementation was chosen among several candidates for its space/time
On 2013/01/04 17:46:24, Andrei Mouravski wrote:
> Since it is an implementation detail, this rationale could go into a non doc
> comment.
> 
> /// Map of tokens and the first associated comment.
> /* 
>  * This implementation...
>  * ...
>  */

Done.

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:132: class TokenKeyMap {
On 2013/01/08 11:59:43, ahe wrote:
> This really is a TokenMap.

Done.

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart
(right):

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:478:
abstract class Dart2JsTypeMirror extends Dart2JsDeclarationMirror
On 2013/01/08 11:59:43, ahe wrote:
> This class defines no behavior, so it has no purpose.
> 
> Chances are that you have code somewhere that says:
> 
> if (m is Dart2JsTypeMirror) doSomething(x);
> 
> Generally, I'd prefer:
> 
> m.doSomething();

It is an 'interface' extending both Dart2JsDeclarationMirror and TypeMirror, and
as such defines their combined behavior.

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:480: 
On 2013/01/08 11:59:43, ahe wrote:
> Extra line.

Done.

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:483:
abstract class Dart2JsElementMirror extends Dart2JsDeclarationMirror {
On 2013/01/08 11:59:43, ahe wrote:
> Generally, I prefer having the super class first. Makes the code easier to
read.

Done.

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart
(right):

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart:137: while
(lines.hasNext) {
On 2013/01/08 11:59:43, ahe wrote:
> lines.moveNext()

Changed to use List and index.

https://codereview.chromium.org/11726005/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart:148: } else
if (lines.hasNext || !line.trim().isEmpty) {
On 2013/01/08 11:59:43, ahe wrote:
> This wont work with the new iterators :-(

Refactored.

https://codereview.chromium.org/11726005/diff/3002/tests/compiler/dart2js/str...
File tests/compiler/dart2js/strip_comment_test.dart (right):

https://codereview.chromium.org/11726005/diff/3002/tests/compiler/dart2js/str...
tests/compiler/dart2js/strip_comment_test.dart:16: testComment('', '// ');
On 2013/01/08 11:59:43, ahe wrote:
> Did you mean:
>  testComment(' ', '//  ');

Yes. Done.

https://codereview.chromium.org/11726005/diff/3002/tests/compiler/dart2js/str...
tests/compiler/dart2js/strip_comment_test.dart:24: testComment('', '/// ');
On 2013/01/08 11:59:43, ahe wrote:
> Ditto?

Done.