Dartdoc and Apidoc updated to use dart2js through the mirror system.

Dartdoc and Apidoc updated to use dart2js through the mirror system.

BUG=3414
Committed: https://code.google.com/p/dart/source/detail?r=9584

[kasperl, 2012-07-06 12:40:41]

LGTM.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:1002: String toString() {
Use => notation?

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1954: SourceString name;
Would it make sense to factor this code (the computation of name) out into a
separate function?

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/comment...
File lib/dartdoc/comment_map.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/comment...
lib/dartdoc/comment_map.dart:48: _comments.putIfAbsent(source.uri().toString(),
() => _parseComments(source));
Line too long (exceeds 80 characters).

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/mirrors...
File lib/dartdoc/mirrors/dart2js_mirror.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/mirrors...
lib/dartdoc/mirrors/dart2js_mirror.dart:108: class Dart2jsMethodKind {
In general, I think I'd prefer calling these Dart2JsXxx (Js rather than js).

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/mirrors...
lib/dartdoc/mirrors/dart2js_mirror.dart:173: // Compiler extension for apidoc
Terminate comment with .

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/utils.dart
File lib/dartdoc/utils.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/utils.d...
lib/dartdoc/utils.dart:60: return transformName(a.simpleName()).compareTo(
Maybe do the transform before calling compareTo and use two extra local
variables?

https://chromiumcodereview.appspot.com/10701091/diff/5001/tools/create_sdk.py
File tools/create_sdk.py (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/tools/create_sdk.py...
tools/create_sdk.py:358: ReplaceInFiles([join(LIB, 'dartdoc', 'frog',
'frog_options.dart')], [
Can you get rid of these 3 replacements (frog related)?

[Lasse Reichstein Nielsen, 2012-07-09 10:39:33]

LGTM

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/classif...
File lib/dartdoc/classify.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/classif...
lib/dartdoc/classify.dart:185: // Color this like a keyword
This isn't generally true any more.
How about:
// Color keyword tokens. Most are colored as keywords.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/classif...
lib/dartdoc/classify.dart:188: // Even though it's a reserved word, let's try
coloring it like a type.
Move this inside the if, so it's obvious that it only refers to the void
keyword. And do, or do not. There is no try.
E.g., write it as
// Color "void" as a type.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/classif...
lib/dartdoc/classify.dart:192: if (token.stringValue === 'this' ||
token.stringValue === 'super') {
// Color "this" and "super" as identifiers.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/comment...
File lib/dartdoc/comment_map.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/comment...
lib/dartdoc/comment_map.dart:82: if (lastComment != null) {
"else if" on one line.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
File lib/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:125: final String libPath = joinPaths(scriptDir,'../');
space after comma.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:158: '${dartdoc._totalTypes} types, and '
Indent to paren.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:190: --generate-app-cache=true   --''--
I'm not sure --''-- is an internationally recognized way of saying 'ditto'.
Replace by "same as --generate-app-cache"
Why is this even here? Can you write "false" too?

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:222: outputDir.createSync();
I never like ignoring an exception.
So you first check if it exists, then deletes it and then recreates it. Sounds
race-condition prone if ever run simultaneously with anything that uses the same
directory, or, heavens forbid, itself.
How about renaming the directory to a temporary name first, then create the new
one, and then delete the old one (asynchroneously). If that deletion fails
(e.g., because someone had a shell open in a subdirectory), it's not a problem
for this process. It can just print a reminder: "Unable to delete directory
blahblah.tmp-11257897125, please remove it manually."

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:263: // TODO(johnniwinther): The following lines get
munged by create-sdk to
How do they get "munged"? Is it just scriptDir/jsPath that are different, or is
something messing with this source code?

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:323: /** Set this to print verbose information during
generation */
/** Set by Dartdoc user to print extra information during generation. */
(I want to know the full life-cycle of public variables, i.e., who may modify
them. In this case it's directly set by the code that creates the Dartdoc
object. 
And then I want to know what difference it makes if it's changed.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:441: dir.createSync();
Could you use
 dir.create().wait()
instead? It's not documented to throw if the directory already exists (but that
might not mean that it doesn't ofcourse).

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:1050: name = 'operator call';
There is no "operator call", nor will there be one. You can remove this code.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:1267: //print('$paramName: $type');
Remove commented code.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:1297: assert (type is InterfaceMirror); // type is a
interface
No space after assert.
"... is an interface", but drop the comment. Asserts *are* comments too.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:1300: // Use .genericType to avoid writing the <...>
here.
Comment out of sync.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:1445: final match = new RegExp(@'new
(\w+)(?:\.(\w+))?').firstMatch(name);
This uses \w for identifiers, which misses "$" as an allowed identifier part. It
should probably be
 @'new ([\w$]+)(?:\.([\w$]+))?'
Ditto below.
(\w matches any ASCII letter or digit, or the underscore character).

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/mirrors...
File lib/dartdoc/mirrors/dart2js_mirror.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/mirrors...
lib/dartdoc/mirrors/dart2js_mirror.dart:260: // we use a specialized resolver
which bypasses method bodies.
Good choice. I expect to see MathNatives go away fairly soon.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/mirrors...
lib/dartdoc/mirrors/dart2js_mirror.dart:340: {
Brace on previous line.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/utils.dart
File lib/dartdoc/utils.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/utils.d...
lib/dartdoc/utils.dart:50: List orderByName(List list) {
Put a generic type on the List types here. It's part of the interface contract. 
It also means I'll actually have a clue what the type of its elements are,
because it isn't mentioned ANYWHERE in this method.

And add types everywhere else too, please!!! It helps reading for me. 
Only ever use "var" when the RHS is a constructor call. Otherwise it is not
clear what type is expected, and I can't read the following code.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/utils.d...
lib/dartdoc/utils.dart:54: }
final elements = new List<whateveritis>.from(list);

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/utils.d...
lib/dartdoc/utils.dart:58: name.toUpperCase().replaceFirst(const
RegExp('\\\$DOM_'), 'zzzz');
Use raw strings for regexp literals: const RegExp(@'\$DOM').

This seems uncredibly slow - you create two new strings *per comparison*. Try:

(a,b) {
  bool doma = a.indexOf(@"$DOM") >= 0;
  bool domb = a.indexOf(@"$DOM") >= 0;
  return doma == domb ? a.compareTo(b) : doma ? 1 : -1;
}

(Or, if the "$DOM" is always at a specific position, you might be able to use
.startsWith instead of .indexOf, or use lastIndexOf it's usually closer to the
end than to the start.

https://chromiumcodereview.appspot.com/10701091/diff/5001/tools/create_sdk.py
File tools/create_sdk.py (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/tools/create_sdk.py...
tools/create_sdk.py:354: ("scriptDir,'../../'",
Space after commas!

https://chromiumcodereview.appspot.com/10701091/diff/5001/tools/create_sdk.py...
tools/create_sdk.py:358: ReplaceInFiles([join(LIB, 'dartdoc', 'frog',
'frog_options.dart')], [
Yes, please!

[Johnni Winther, 2012-07-09 14:57:18]

Merged and fixed cf. comments.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:1002: String toString() {
On 2012/07/06 12:40:41, kasperl wrote:
> Use => notation?

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1954: SourceString name;
On 2012/07/06 12:40:41, kasperl wrote:
> Would it make sense to factor this code (the computation of name) out into a
> separate function? 

For clarity only. I don't think the computation is needed elsewhere.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/classif...
File lib/dartdoc/classify.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/classif...
lib/dartdoc/classify.dart:185: // Color this like a keyword
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> This isn't generally true any more.
> How about:
> // Color keyword tokens. Most are colored as keywords.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/classif...
lib/dartdoc/classify.dart:188: // Even though it's a reserved word, let's try
coloring it like a type.
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> Move this inside the if, so it's obvious that it only refers to the void
> keyword. And do, or do not. There is no try.
> E.g., write it as
> // Color "void" as a type.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/classif...
lib/dartdoc/classify.dart:192: if (token.stringValue === 'this' ||
token.stringValue === 'super') {
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> // Color "this" and "super" as identifiers.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/comment...
File lib/dartdoc/comment_map.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/comment...
lib/dartdoc/comment_map.dart:48: _comments.putIfAbsent(source.uri().toString(),
() => _parseComments(source));
On 2012/07/06 12:40:41, kasperl wrote:
> Line too long (exceeds 80 characters).

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/comment...
lib/dartdoc/comment_map.dart:82: if (lastComment != null) {
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> "else if" on one line.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
File lib/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:125: final String libPath = joinPaths(scriptDir,'../');
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> space after comma.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:158: '${dartdoc._totalTypes} types, and '
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> Indent to paren.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:190: --generate-app-cache=true   --''--
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> I'm not sure --''-- is an internationally recognized way of saying 'ditto'.
> Replace by "same as --generate-app-cache"
> Why is this even here? Can you write "false" too? 

You can't write --generate-app-cache=false. I think we should just remove the
documentation for --generate-app-cache=true but leave the implementation in case
anyone should still rely on it. The '=true' option syntax seems to be inherited
from apidoc.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:222: outputDir.createSync();
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> I never like ignoring an exception.
> So you first check if it exists, then deletes it and then recreates it. Sounds
> race-condition prone if ever run simultaneously with anything that uses the
same
> directory, or, heavens forbid, itself.
> How about renaming the directory to a temporary name first, then create the
new
> one, and then delete the old one (asynchroneously). If that deletion fails
> (e.g., because someone had a shell open in a subdirectory), it's not a problem
> for this process. It can just print a reminder: "Unable to delete directory
> blahblah.tmp-11257897125, please remove it manually."

I will fix it later.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:263: // TODO(johnniwinther): The following lines get
munged by create-sdk to
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> How do they get "munged"? Is it just scriptDir/jsPath that are different, or
is
> something messing with this source code?
Yes, create_sdk.py is!

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:323: /** Set this to print verbose information during
generation */
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> /** Set by Dartdoc user to print extra information during generation. */
> (I want to know the full life-cycle of public variables, i.e., who may modify
> them. In this case it's directly set by the code that creates the Dartdoc
> object. 
> And then I want to know what difference it makes if it's changed.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:441: dir.createSync();
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> Could you use
>  dir.create().wait()
> instead? It's not documented to throw if the directory already exists (but
that
> might not mean that it doesn't ofcourse).

I will fix it later.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:1050: name = 'operator call';
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> There is no "operator call", nor will there be one. You can remove this code.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:1267: //print('$paramName: $type');
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> Remove commented code.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:1297: assert (type is InterfaceMirror); // type is a
interface
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> No space after assert.
> "... is an interface", but drop the comment. Asserts *are* comments too.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:1300: // Use .genericType to avoid writing the <...>
here.
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> Comment out of sync.

It doesn't apply anymore.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:1445: final match = new RegExp(@'new
(\w+)(?:\.(\w+))?').firstMatch(name);
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> This uses \w for identifiers, which misses "$" as an allowed identifier part.
It
> should probably be
>  @'new ([\w$]+)(?:\.([\w$]+))?'
> Ditto below.
> (\w matches any ASCII letter or digit, or the underscore character).

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/mirrors...
File lib/dartdoc/mirrors/dart2js_mirror.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/mirrors...
lib/dartdoc/mirrors/dart2js_mirror.dart:108: class Dart2jsMethodKind {
On 2012/07/06 12:40:41, kasperl wrote:
> In general, I think I'd prefer calling these Dart2JsXxx (Js rather than js).

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/mirrors...
lib/dartdoc/mirrors/dart2js_mirror.dart:173: // Compiler extension for apidoc
On 2012/07/06 12:40:41, kasperl wrote:
> Terminate comment with .

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/mirrors...
lib/dartdoc/mirrors/dart2js_mirror.dart:340: {
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> Brace on previous line.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/utils.dart
File lib/dartdoc/utils.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/utils.d...
lib/dartdoc/utils.dart:50: List orderByName(List list) {
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> Put a generic type on the List types here. It's part of the interface
contract. 
> It also means I'll actually have a clue what the type of its elements are,
> because it isn't mentioned ANYWHERE in this method.
> 
> And add types everywhere else too, please!!! It helps reading for me. 
> Only ever use "var" when the RHS is a constructor call. Otherwise it is not
> clear what type is expected, and I can't read the following code.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/utils.d...
lib/dartdoc/utils.dart:54: }
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> final elements = new List<whateveritis>.from(list);

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/utils.d...
lib/dartdoc/utils.dart:58: name.toUpperCase().replaceFirst(const
RegExp('\\\$DOM_'), 'zzzz');
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> Use raw strings for regexp literals: const RegExp(@'\$DOM').
> 
> This seems uncredibly slow - you create two new strings *per comparison*. Try:
> 
> (a,b) {
>   bool doma = a.indexOf(@"$DOM") >= 0;
>   bool domb = a.indexOf(@"$DOM") >= 0;
>   return doma == domb ? a.compareTo(b) : doma ? 1 : -1;
> }
> 
> (Or, if the "$DOM" is always at a specific position, you might be able to use
> .startsWith instead of .indexOf, or use lastIndexOf it's usually closer to the
> end than to the start.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/lib/dartdoc/utils.d...
lib/dartdoc/utils.dart:60: return transformName(a.simpleName()).compareTo(
On 2012/07/06 12:40:41, kasperl wrote:
> Maybe do the transform before calling compareTo and use two extra local
> variables?

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/tools/create_sdk.py
File tools/create_sdk.py (right):

https://chromiumcodereview.appspot.com/10701091/diff/5001/tools/create_sdk.py...
tools/create_sdk.py:354: ("scriptDir,'../../'",
On 2012/07/09 10:39:33, Lasse Reichstein Nielsen wrote:
> Space after commas!

Done.

https://chromiumcodereview.appspot.com/10701091/diff/5001/tools/create_sdk.py...
tools/create_sdk.py:358: ReplaceInFiles([join(LIB, 'dartdoc', 'frog',
'frog_options.dart')], [
On 2012/07/06 12:40:41, kasperl wrote:
> Can you get rid of these 3 replacements (frog related)?

They will be removed together with the dartdoc/frog/ folder.

[Bob Nystrom, 2012-07-09 16:59:55]

This is very cool. It's going to be awesome having a single unified mirrors API
that we can use for this and stuff at runtime.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/compiler/imple...
File lib/compiler/implementation/apiimpl.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/compiler/imple...
lib/compiler/implementation/apiimpl.dart:109: // instance in the [Types]
constructor in typechecker.dart.
I find myself doing this too out of habit, but, for what it's worth, there's
little point in using dartdoc formatting in non-doc comments like this. They'll
never be processed. I suppose if we ever make a literate programming version of
dartdoc that does process in-source comments, maybe this would be useful...

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/compiler/imple...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/compiler/imple...
lib/compiler/implementation/compiler.dart:1053: String toString() =>
'SourceSpan($uri,$begin,$end)';
Spaces after ",".

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/classi...
File lib/dartdoc/classify.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/classi...
lib/dartdoc/classify.dart:7:
#import('../compiler/implementation/scanner/scannerlib.dart');
One of the things we've been discussing for the style guide is library naming
conventions. Part of that will be to "not" use "lib" in the name of a library,
since it's redundant. Can you remove this?

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/classi...
lib/dartdoc/classify.dart:63: html.add('<span>$escapedText</span>');
Would this work if you just add the text directly instead of inside a span? It
isn't being styled, so it seems like the span is useless.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
File lib/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:164: void printUsage() {
This is very nice, but at some point (not in this patch), it would be cool if
dartdoc used the existing args library for this.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:270: return true;
Why have this return value if it's always true?

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:329: * From exposes the set of libraries in
`world.libraries`. That maps library
This comment probably needs updating now.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:734: //if (type.isTop) continue;
Just delete this if it's dead code.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:840: = new List.from(types).filter((t) =>
!t.isPrivate);
Style nit. I would wrap this after filter(

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:913: * including getters, setters, and constructors.
Fix this comment.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:1123: // For now, we'll assume you don't do that.
You can remove this comment now.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:1279: // Do not generate links for void
"." at end of comment.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:1331: }
The style guide allows single-line ifs for things like this, if you prefer:

if (type.isVoid) return 'void';

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:1433: final match = new RegExp(@'new
([\w$]+)(?:\.([\w$]+))?').firstMatch(name);
Long line.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:1476: int getSpanColumn(Location span) {
This seems like something Location should support directly.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/mirror...
File lib/dartdoc/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/mirror...
lib/dartdoc/mirrors/mirrors.dart:34: abstract MirrorSystem mirrors();
I think most of these should be getters.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/mirror...
lib/dartdoc/mirrors/mirrors.dart:44: Map<Object, LibraryMirror> libraries();
Ditto...

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/mirror...
lib/dartdoc/mirrors/mirrors.dart:57: String simpleName();
Especially this one...

https://chromiumcodereview.appspot.com/10701091/diff/11001/utils/apidoc/apido...
File utils/apidoc/apidoc.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/11001/utils/apidoc/apido...
utils/apidoc/apidoc.dart:120: ];
Style nit: indent just +2, not +4 and don't indent the trailing "]". Like:

var apidocLibraries = <String>[
  'dart:core',
  'dart:coreimpl'
];

https://chromiumcodereview.appspot.com/10701091/diff/11001/utils/apidoc/apido...
utils/apidoc/apidoc.dart:138: ];
Ditto.

https://chromiumcodereview.appspot.com/10701091/diff/11001/utils/apidoc/apido...
utils/apidoc/apidoc.dart:152: /**
Style nit. Can you put a blank line between doc comments and the preceding
definition?

[Johnni Winther, 2012-07-12 08:51:39]

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/compiler/imple...
File lib/compiler/implementation/apiimpl.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/compiler/imple...
lib/compiler/implementation/apiimpl.dart:109: // instance in the [Types]
constructor in typechecker.dart.
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> I find myself doing this too out of habit, but, for what it's worth, there's
> little point in using dartdoc formatting in non-doc comments like this.
They'll
> never be processed. I suppose if we ever make a literate programming version
of
> dartdoc that does process in-source comments, maybe this would be useful...

The [:...:] syntax doesn't look good, but the [...] actually makes comments more
readable since they easily mark when a word a code entity.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/compiler/imple...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/compiler/imple...
lib/compiler/implementation/compiler.dart:1053: String toString() =>
'SourceSpan($uri,$begin,$end)';
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> Spaces after ",".

Done.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/classi...
File lib/dartdoc/classify.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/classi...
lib/dartdoc/classify.dart:7:
#import('../compiler/implementation/scanner/scannerlib.dart');
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> One of the things we've been discussing for the style guide is library naming
> conventions. Part of that will be to "not" use "lib" in the name of a library,
> since it's redundant. Can you remove this?

Well, we have a name-clash with the scanner.dart file, but if we rename that as
well...

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/classi...
lib/dartdoc/classify.dart:63: html.add('<span>$escapedText</span>');
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> Would this work if you just add the text directly instead of inside a span? It
> isn't being styled, so it seems like the span is useless.

Span removed.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
File lib/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:164: void printUsage() {
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> This is very nice, but at some point (not in this patch), it would be cool if
> dartdoc used the existing args library for this.

Yes.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:270: return true;
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> Why have this return value if it's always true?

Legacy code. Return type changed to void.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:329: * From exposes the set of libraries in
`world.libraries`. That maps library
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> This comment probably needs updating now.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:734: //if (type.isTop) continue;
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> Just delete this if it's dead code.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:840: = new List.from(types).filter((t) =>
!t.isPrivate);
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> Style nit. I would wrap this after filter(

Done.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:913: * including getters, setters, and constructors.
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> Fix this comment.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:1123: // For now, we'll assume you don't do that.
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> You can remove this comment now.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:1279: // Do not generate links for void
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> "." at end of comment.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:1433: final match = new RegExp(@'new
([\w$]+)(?:\.([\w$]+))?').firstMatch(name);
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> Long line.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/dartdo...
lib/dartdoc/dartdoc.dart:1476: int getSpanColumn(Location span) {
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> This seems like something Location should support directly.

Moved to mirrors_util.dart instead. We haven't settled on the the Location
interface yet.

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/mirror...
File lib/dartdoc/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/dartdoc/mirror...
lib/dartdoc/mirrors/mirrors.dart:34: abstract MirrorSystem mirrors();
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> I think most of these should be getters.

Yes. This is general issue which will be resolved for the whole mirror system
later.

https://chromiumcodereview.appspot.com/10701091/diff/11001/utils/apidoc/apido...
File utils/apidoc/apidoc.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/11001/utils/apidoc/apido...
utils/apidoc/apidoc.dart:120: ];
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> Style nit: indent just +2, not +4 and don't indent the trailing "]". Like:
> 
> var apidocLibraries = <String>[
>   'dart:core',
>   'dart:coreimpl'
> ];

Done.

https://chromiumcodereview.appspot.com/10701091/diff/11001/utils/apidoc/apido...
utils/apidoc/apidoc.dart:138: ];
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10701091/diff/11001/utils/apidoc/apido...
utils/apidoc/apidoc.dart:152: /**
On 2012/07/09 16:59:55, Bob Nystrom wrote:
> Style nit. Can you put a blank line between doc comments and the preceding
> definition?

Done.

[Bob Nystrom, 2012-07-12 16:42:30]

LGTM!

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/compiler/imple...
File lib/compiler/implementation/apiimpl.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/11001/lib/compiler/imple...
lib/compiler/implementation/apiimpl.dart:109: // instance in the [Types]
constructor in typechecker.dart.
On 2012/07/12 08:51:39, Johnni Winther wrote:
> On 2012/07/09 16:59:55, Bob Nystrom wrote:
> > I find myself doing this too out of habit, but, for what it's worth, there's
> > little point in using dartdoc formatting in non-doc comments like this.
> They'll
> > never be processed. I suppose if we ever make a literate programming version
> of
> > dartdoc that does process in-source comments, maybe this would be useful...
> 
> The [:...:] syntax doesn't look good, but the [...] actually makes comments
more
> readable since they easily mark when a word a code entity.

True on []. Also, since dartdoc is markdown, you can also use backticks instead
of [:...:].

[ahe, 2012-08-02 19:56:22]

https://chromiumcodereview.appspot.com/10701091/diff/5003/lib/compiler/implem...
File lib/compiler/implementation/apiimpl.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5003/lib/compiler/implem...
lib/compiler/implementation/apiimpl.dart:108: // [:span.uri:] might be [:null:]
in case of a [Script] with no [uri]. For
Actually, span.uri may not be null...

https://chromiumcodereview.appspot.com/10701091/diff/5003/lib/compiler/implem...
lib/compiler/implementation/apiimpl.dart:109: // instance in the [Types]
constructor in typechecker.dart.
... because Types shouldn't do that :-)

https://chromiumcodereview.appspot.com/10701091/diff/5003/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5003/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:980: if (uri === null &&
currentElement !== null) {
currentElement may not be null.

https://chromiumcodereview.appspot.com/10701091/diff/5003/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10701091/diff/5003/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:156: void visitBody(ResolverVisitor
visitor, Statement body) {
What is the purpose of this method?