Rewrite the pkg/yaml parser.

Rewrite the pkg/yaml parser.

This increases the speed by about 10x. It also adds a number of new featurse.

BUG=21317, 20859
R=rnystrom@google.com

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

[nweiz, 2014-10-29 00:56:01]

nweiz@google.com changed reviewers:
+ rnystrom@google.com

[nweiz, 2014-10-29 00:56:03]

Ignore the pkg/collection changes, they're from a precursor CL.

[Bob Nystrom, 2014-10-31 20:03:30]

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/event.dart
File pkg/yaml/lib/src/event.dart (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/event.d...
pkg/yaml/lib/src/event.dart:27: final type = EventType.DOCUMENT_START;
It's more efficient to use getters, here and below.

See this for details: https://gist.github.com/munificent/b8f2785c7d1744bdd22f

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/event.d...
pkg/yaml/lib/src/event.dart:38: final bool implicit;
Maybe "isImplicit"?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/event.d...
pkg/yaml/lib/src/event.dart:44: String toString() => "DOCUMENT_START";
Why not just extend Event and use the inherited toString()?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/loader....
File pkg/yaml/lib/src/loader.dart (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/loader....
pkg/yaml/lib/src/loader.dart:22: class Loader {
The separation between Loader and Parser is interesting. I can see it making
sense in C maybe because you don't have exceptions? Is there another rationale
behind it?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/loader....
pkg/yaml/lib/src/loader.dart:33: /// Creates a parser that loads [source].
"loader".

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/loader....
pkg/yaml/lib/src/loader.dart:45: if (_parser.isDone) return null;
Document this.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/loader....
pkg/yaml/lib/src/loader.dart:117: node = _parseByTag(scalar);
I think it would read a little clearer to do this case first:

if (scalar.tag != null) {
  node = _parseByTag(scalar);
} ...

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/loader....
pkg/yaml/lib/src/loader.dart:204: var match = new
RegExp(r"^[-+]?[0-9]+$").firstMatch(scalar.value);
Is "0000123" allowed?

I wonder if it's better to just do:

try {
  int.parse...
} on FormatException catch (error) { ... }

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/parser....
File pkg/yaml/lib/src/parser.dart (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/parser....
pkg/yaml/lib/src/parser.dart:73: case _State.BLOCK_SEQUENCE_FIRST_ENTRY:
How about just putting _scanner.scan() here and removing the "first" param?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/parser....
pkg/yaml/lib/src/parser.dart:218: return new DocumentEndEvent(span, implicit:
implicit);
How about just:

Event _parseDocumentEnd() {
  _tagDirectives.clear();
  _state = _State.DOCUMENT_START;

  var token = _scanner.peek();
  if (token.type == TokenType.DOCUMENT_END) {
    return new DocumentEndEvent(token.span, implicit: false);
  } else {
    return new DocumentEndEvent(token.span.start.pointSpan(), implicit: true);
  }
}

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/parser....
pkg/yaml/lib/src/parser.dart:260: if (token is AnchorToken) {
Lot of copy/paste here. How about local functions for parseAnchor() and
parseTag()?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/parser....
pkg/yaml/lib/src/parser.dart:368: token = _scanner.peek();
This two line pattern is really common. I assume .scan() returns the previous
token. I wonder if it's worth adding something like .advance() to StringScanner
that moves forward and returns the next token?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/parser....
pkg/yaml/lib/src/parser.dart:514: 
Extra blank line.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
File pkg/yaml/lib/src/scanner.dart (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:26: static const TILDE = 0x7E;
Move to after GRAVE_ACCENT?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:27: static const NEL = 0x85;
This one's a bit uncommon. How about moving either to the next section or have
it in numeric order after GRAVE_ACCENT.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:109: var _flowLevel = 0;
Can this be inferred from _simpleKeys.length?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:121: var _tokensParsed = 0;
"Parsed" -> "Scanned"?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:130: final _indents = new List<int>();
<int>[]

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:133: var _indent = -1;
Document what -1 means (or make a constant).

Does this need to be a separate field, or could this be _indents.last and just
push the value onto that instead of setting this?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:148: final _simpleKeys = <_SimpleKey>[null];
Why isn't this initially empty?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:180: char == RIGHT_PAREN || char == PERCENT;
It may be quicker to look this up in a map or even index into a bool array.
Maybe see if that makes a noticeable perf difference?

Another option is to use a switch for the literal values like you do in
_isNonBreak. That may let the VM optimize it to a jump table for you.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:231: ((char >= 0x00020 && char <= 0x00007E) ||
Nit: +2 more before "(".

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:244: return char != null && char != LF && char !=
CR && char != BOM && char != SP &&
Long line.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:250: (char >= 0x10000 && char <= 0x10FFFF));
This expression is pretty huge. How about using a switch or some early returns
like the above fns?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:284: if (!_simpleKeys.any((key) =>
Document this.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:315: } else if (_isBlankOrEndAt(3)) {
Ditch the else.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:319: } else if (_scanner.matches('...')) {
Here too.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:329: return;
Is there a reason to prefer return over break throughout this?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:429: //
(http://yaml.org/spec/1.2/spec.html#id2790832), these restriction is
"restrictions are"

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:492: /// Pushes the current indentation level to
the stack and sets the new level if
Long line.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:495: /// In it is, appends or inserts the
specified token into [_tokens]. If
"it is"?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:566: _simpleKeyAllowed = false;
Hoist these three lines into a _resetState() method?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:607: "Block sequence entries are not allowed in
this context.",
Would be good to describe the context instead of just "this".

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:630: throw new YamlException("Mapping keys are not
allowed in this context.",
Ditto.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:799: warn("Warning: unknown directive.",
_scanner.spanFrom(start));
I don't think the parser should output directly to the terminal. How about
having this invoke a user-provided callback or at least be configurable in some
way?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:813: }
Make a _skipBlanks() method for this since you do it pretty frequently.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:819: }
Probably this too.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:827: if (_isBreak) _skipLine();
Do you need to check _isBreak here? Doesn't _skipLine() do that?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:842: buffer.writeCharCode(_scanner.readChar());
This seems inefficient. Can you just get a substring directly from the _scanner?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:848: } else if (!_isBlankOrEnd) {
What about:

%YAML#Comment.

I'd expect this to be a parse error, but not necessarily a scanner error. Maybe
just look for a hash and throw a different error?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:936: next != PERCENT && next != AT           &&
next != GRAVE_ACCENT)) {
What are these specific character tests for?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:955: if (_scanner.peekChar(1) == LEFT_ANGLE) {
Does this fail on "!" (a bang by itself)?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:1067: if (_scanner.peekChar() == 0) {
NUMBER_0?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:1077: if (_scanner.peekChar() == 0) {
Ditto.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:1111: var indent = 0;
Document this little block.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:1146: leadingBreak = '';
Move this after the if.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:1174: if (chomping != _Chomping.STRIP) {
Nit, but maybe make these single-line ifs?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:1234: }
Hoist this out into a function?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:1274: case TAB:
Oh, YAML. You so crazy.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/token.dart
File pkg/yaml/lib/src/token.dart (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/token.d...
pkg/yaml/lib/src/token.dart:37: String toString() =>
"TokenType.VERSION_DIRECTIVE $major.$minor";
Remove "TokenType.".

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/token.d...
pkg/yaml/lib/src/token.dart:69: /// A token representing an alias (`*foo`)
"."

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/yaml_do...
File pkg/yaml/lib/src/yaml_document.dart (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/yaml_do...
pkg/yaml/lib/src/yaml_document.dart:25: final List<TagDirective> tagDirectives;
Maybe just "tags"? Are there non-directive tags?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/yaml_no...
File pkg/yaml/lib/src/yaml_node.dart (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node.dart:86: _span = span;
Put this in the initialization list.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node.dart:136: _span = span;
Ditto.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node.dart:175: /// This method is not exposed publicly.
If you're using this pattern here, do you think it's worth using it for the
internal constructors too?

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/libyaml-license...
File pkg/yaml/libyaml-license.txt (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/libyaml-license...
pkg/yaml/libyaml-license.txt:1: Copyright (c) 2006 Kirill Simonov
I'm a license noob. Can you check with Seth or Kevin and make sure this is OK?

[nweiz, 2014-11-04 01:43:37]

Code review changes

[nweiz, 2014-11-04 22:19:34]

nweiz@google.com changed reviewers:
+ kevmoo@google.com, sethladd@google.com

[nweiz, 2014-11-04 22:19:38]

+sethladd +kevmoo to make sure I'm doing the right thing with the license here.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/event.dart
File pkg/yaml/lib/src/event.dart (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/event.d...
pkg/yaml/lib/src/event.dart:27: final type = EventType.DOCUMENT_START;
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> It's more efficient to use getters, here and below.
> 
> See this for details: https://gist.github.com/munificent/b8f2785c7d1744bdd22f

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/event.d...
pkg/yaml/lib/src/event.dart:38: final bool implicit;
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> Maybe "isImplicit"?

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/event.d...
pkg/yaml/lib/src/event.dart:44: String toString() => "DOCUMENT_START";
On 2014/10/31 20:03:26, Bob Nystrom wrote:
> Why not just extend Event and use the inherited toString()?

Because then we'd have to pass a type to its superclass constructor.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/loader....
File pkg/yaml/lib/src/loader.dart (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/loader....
pkg/yaml/lib/src/loader.dart:22: class Loader {
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> The separation between Loader and Parser is interesting. I can see it making
> sense in C maybe because you don't have exceptions? Is there another rationale
> behind it?

It might make sense to expose the parser on its own at some point to provide
access to the raw tags as they exist in the document. It's also a separation of
concerns, so that the parser can focus entirely on making sense of the scanner's
tokens and not on constructing aliases. Mostly it just makes the port more
direct, though.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/loader....
pkg/yaml/lib/src/loader.dart:33: /// Creates a parser that loads [source].
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> "loader".

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/loader....
pkg/yaml/lib/src/loader.dart:45: if (_parser.isDone) return null;
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> Document this.

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/loader....
pkg/yaml/lib/src/loader.dart:117: node = _parseByTag(scalar);
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> I think it would read a little clearer to do this case first:
> 
> if (scalar.tag != null) {
>   node = _parseByTag(scalar);
> } ...

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/loader....
pkg/yaml/lib/src/loader.dart:204: var match = new
RegExp(r"^[-+]?[0-9]+$").firstMatch(scalar.value);
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> Is "0000123" allowed?

Yes.

> I wonder if it's better to just do:
> 
> try {
>   int.parse...
> } on FormatException catch (error) { ... }

I imagine the Dart parsing rules aren't identical to the Yaml parsing rules.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/parser....
File pkg/yaml/lib/src/parser.dart (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/parser....
pkg/yaml/lib/src/parser.dart:73: case _State.BLOCK_SEQUENCE_FIRST_ENTRY:
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> How about just putting _scanner.scan() here and removing the "first" param?

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/parser....
pkg/yaml/lib/src/parser.dart:218: return new DocumentEndEvent(span, implicit:
implicit);
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> How about just:
> 
> Event _parseDocumentEnd() {
>   _tagDirectives.clear();
>   _state = _State.DOCUMENT_START;
> 
>   var token = _scanner.peek();
>   if (token.type == TokenType.DOCUMENT_END) {
>     return new DocumentEndEvent(token.span, implicit: false);
>   } else {
>     return new DocumentEndEvent(token.span.start.pointSpan(), implicit: true);
>   }
> }

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/parser....
pkg/yaml/lib/src/parser.dart:260: if (token is AnchorToken) {
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> Lot of copy/paste here. How about local functions for parseAnchor() and
> parseTag()?

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/parser....
pkg/yaml/lib/src/parser.dart:368: token = _scanner.peek();
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> This two line pattern is really common. I assume .scan() returns the previous
> token. I wonder if it's worth adding something like .advance() to
StringScanner
> that moves forward and returns the next token?

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/parser....
pkg/yaml/lib/src/parser.dart:514: 
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> Extra blank line.

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
File pkg/yaml/lib/src/scanner.dart (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:26: static const TILDE = 0x7E;
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> Move to after GRAVE_ACCENT?

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:27: static const NEL = 0x85;
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> This one's a bit uncommon. How about moving either to the next section or have
> it in numeric order after GRAVE_ACCENT.

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:109: var _flowLevel = 0;
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> Can this be inferred from _simpleKeys.length?

Yes, good idea.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:121: var _tokensParsed = 0;
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> "Parsed" -> "Scanned"?

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:130: final _indents = new List<int>();
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> <int>[]

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:133: var _indent = -1;
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> Document what -1 means (or make a constant).

Done.

> Does this need to be a separate field, or could this be _indents.last and just
> push the value onto that instead of setting this?

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:148: final _simpleKeys = <_SimpleKey>[null];
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> Why isn't this initially empty?

Because there is an initial flow level that could have a simple key.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:180: char == RIGHT_PAREN || char == PERCENT;
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> It may be quicker to look this up in a map or even index into a bool array.
> Maybe see if that makes a noticeable perf difference?
> 
> Another option is to use a switch for the literal values like you do in
> _isNonBreak. That may let the VM optimize it to a jump table for you.

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:231: ((char >= 0x00020 && char <= 0x00007E) ||
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> Nit: +2 more before "(".

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:244: return char != null && char != LF && char !=
CR && char != BOM && char != SP &&
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:250: (char >= 0x10000 && char <= 0x10FFFF));
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> This expression is pretty huge. How about using a switch or some early returns
> like the above fns?

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:284: if (!_simpleKeys.any((key) =>
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> Document this.

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:315: } else if (_isBlankOrEndAt(3)) {
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> Ditch the else.

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:319: } else if (_scanner.matches('...')) {
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> Here too.

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:329: return;
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> Is there a reason to prefer return over break throughout this?

It allows the reader to avoid checking the end of the function to see if
anything else happens while following the flow.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:429: //
(http://yaml.org/spec/1.2/spec.html#id2790832), these restriction is
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> "restrictions are"

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:492: /// Pushes the current indentation level to
the stack and sets the new level if
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:495: /// In it is, appends or inserts the
specified token into [_tokens]. If
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> "it is"?

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:566: _simpleKeyAllowed = false;
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> Hoist these three lines into a _resetState() method?

I'd rather have the visual similarity with the methods below that don't have the
exact same lines to make the contrast explicit.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:607: "Block sequence entries are not allowed in
this context.",
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> Would be good to describe the context instead of just "this".

That's pretty tough... we'd have to track the reason that [_simpleKeyAllowed]
was set to false. I think the description combined with the span information
should be sufficient.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:799: warn("Warning: unknown directive.",
_scanner.spanFrom(start));
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> I don't think the parser should output directly to the terminal. How about
> having this invoke a user-provided callback or at least be configurable in
some
> way?

Done. I wish there were a more standard way to do this.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:813: }
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> Make a _skipBlanks() method for this since you do it pretty frequently.

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:819: }
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> Probably this too.

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:827: if (_isBreak) _skipLine();
On 2014/10/31 20:03:27, Bob Nystrom wrote:
> Do you need to check _isBreak here? Doesn't _skipLine() do that?

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:842: buffer.writeCharCode(_scanner.readChar());
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> This seems inefficient. Can you just get a substring directly from the
_scanner?

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:848: } else if (!_isBlankOrEnd) {
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> What about:
> 
> %YAML#Comment.
> 
> I'd expect this to be a parse error, but not necessarily a scanner error.
Maybe
> just look for a hash and throw a different error?

I don't think that's a likely enough error to warrant special-casing the
message.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:936: next != PERCENT && next != AT           &&
next != GRAVE_ACCENT)) {
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> What are these specific character tests for?

They check whether the anchor is followed by some sort of flow indicator, which
is allowed (it indicates that the anchor is anchoring an empty scalar).
Otherwise, it's followed by a character that's invalid when adjacent to an
anchor—possibly an invalid text character, possibly something like "&anchor[]".

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:955: if (_scanner.peekChar(1) == LEFT_ANGLE) {
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> Does this fail on "!" (a bang by itself)?

No; [peekChar] returns null for out-of-range indices for just this reason.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:1067: if (_scanner.peekChar() == 0) {
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> NUMBER_0?

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:1077: if (_scanner.peekChar() == 0) {
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> Ditto.

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:1111: var indent = 0;
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> Document this little block.

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:1146: leadingBreak = '';
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> Move this after the if.

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:1174: if (chomping != _Chomping.STRIP) {
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> Nit, but maybe make these single-line ifs?

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/scanner...
pkg/yaml/lib/src/scanner.dart:1234: }
On 2014/10/31 20:03:28, Bob Nystrom wrote:
> Hoist this out into a function?

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/token.dart
File pkg/yaml/lib/src/token.dart (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/token.d...
pkg/yaml/lib/src/token.dart:37: String toString() =>
"TokenType.VERSION_DIRECTIVE $major.$minor";
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> Remove "TokenType.".

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/token.d...
pkg/yaml/lib/src/token.dart:69: /// A token representing an alias (`*foo`)
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> "."

Done.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/yaml_do...
File pkg/yaml/lib/src/yaml_document.dart (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/yaml_do...
pkg/yaml/lib/src/yaml_document.dart:25: final List<TagDirective> tagDirectives;
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> Maybe just "tags"? Are there non-directive tags?

Yes; the things that are attached to actual YAML values (e.g. "!tagname value").

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/yaml_no...
File pkg/yaml/lib/src/yaml_node.dart (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node.dart:86: _span = span;
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> Put this in the initialization list.

Dart won't allow it. Apparently you can't hop inheritance chains in
initialization lists. I guess this is because otherwise changing the superclass
from "var _span" to "set _span" would be a breaking change.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/lib/src/yaml_no...
pkg/yaml/lib/src/yaml_node.dart:175: /// This method is not exposed publicly.
On 2014/10/31 20:03:29, Bob Nystrom wrote:
> If you're using this pattern here, do you think it's worth using it for the
> internal constructors too?

Nope. Constructors are more complicated to wrap and easier to clearly mark as
internal.

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/libyaml-license...
File pkg/yaml/libyaml-license.txt (right):

https://codereview.chromium.org/689513002/diff/20001/pkg/yaml/libyaml-license...
pkg/yaml/libyaml-license.txt:1: Copyright (c) 2006 Kirill Simonov
On 2014/10/31 20:03:30, Bob Nystrom wrote:
> I'm a license noob. Can you check with Seth or Kevin and make sure this is OK?

Done.

[sethladd, 2014-11-04 22:32:31]

I wish I could help, but I would start by asking oss-discuss@google.com.

[Bob Nystrom, 2014-11-04 23:15:53]

LGTM. Really nice!

[nweiz, 2014-11-10 21:49:48]

Committed patchset #3 (id:40001) manually as 41649 (presubmit successful).