YAML Parser

[klimek, 2012-02-02 10:19:21]

First run over the header. Generally, the tooling code doesn't change too much
with the new interface, so it's definitely in an "I can live with it" state.

One main high-level design question: Do we really need all the implemented YAML
features?

Some general design things we probably just have different opinions about, which
I ran into while changing the tooling code to use the YAMLParser. Feel free to
strongly object :)
- I find it easier as a user to have the public section as first section in a
class definition; you seem to have a mixture, which I find a little confusing
- const iterators are missing
- I find references to higher-level scope objects as members in a class to be
bug-prone (you cannot see that the reference is kept from the call site)
- I'm a strong opponent of abbrs; I'm not sure whether as a non-native speaker
it's just harder to read them or I'm just obsessive, but I'm a big fan of
spending the 0.01 seconds it takes to spell out Current or Position.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser.h
File include/llvm/Support/YAMLParser.h (right):

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:18: //    * BOMs anywhere other than the first
Unicode scalar value in the file.
It would be cool to have a pointer to the first class I have to look at as a
user (Stream). In general, I'm wondering whether we can reduce the size of the
visible interface here...

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:44: UEF_UTF32_LE,
Perhaps document LE vs. BE...

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:64: struct StreamStartInfo {
Is this only used for the scanning? If so, would it make sense to split up the
header? Why's this a struct instead of a typedef? (do you intend to extend
this?) Some documentation might help. Same for the next 2 structs.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:77: struct Token : ilist_node<Token> {
Are Tokens meant to be changed after being created? If not, I'd vote for a
non-modifiable design.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:103: /// A string of length 0 or more who's
begin() points to the logical location
s/who's/whose/

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:108: ///       can't be in a union in C++03.
Why not use inheritance instead?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:155: /// Simple keys are handled by creating
an entry in SimpleKeys for each Token
I don't find the concept of a "simple key" in the YAML spec - I think it would
be helpful to reference the standard.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:161: /// others) before the SimpleKey's Tok.
In general, this comment doesn't really help me understand the concept. It
basically explains some coupling, but that looks pretty random when I'm
contextually at this part of the file.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:162: struct SimpleKey {
At a first glance this looks like an internal structure used for parsing - can
we hide this?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:177: Scanner(const StringRef input, SourceMgr
&sm);
Style: variables start with upper case letter (or is there a special rule I'm
missing?)

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:191: if (Pos >= End)
Why can that happen?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:215: bool isAtEnd(StringRef::iterator i = 0) {
Unused?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:233: /// @brief Skip a single nb-char[27]
starting at Pos.
What does 'nb' stand for here? Same for the functions below, I have no idea what
those initializations mean.

Also, here and elsewhere - are those bracketed numbers referring to
documentation somewhere?

And lastly, style: shouldn't methods be camelCase?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:262: /// @brief Skip minimal well-formed code
unit subsequence's until Func
s/'//?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:270: StringRef::iterator i =
(this->*Func)(Pos);
Style: capital letter for variables?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:304: /// @brief Remove simple keys that can no
longer be valid simple keys.
If they can no longer be valid simple keys, perhaps call this
removeStaleSimpleKeyCandidates?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:319: bool rollIndent( int Col
Is the indent here intentional?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:413: TokenQueueT TokenQueue;
If this only holds a limited number of tokens (as simple keys are limited to
1024 tokens if I understand it correctly), would using a SmallVector lead to the
same performance with less code?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:422: class document_iterator;
From your naming I assume that using lower_case for a class is correct style for
iterators?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:437: Stream(StringRef input, SourceMgr &sm);
Capitalization of variables?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:467: Node(unsigned int Type, Document *D,
StringRef A);
It's really hard to guess what 'A' stands for here without reading the context -
can we call it Anchor?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:477: // These functions forward to Document
and Scanner.
This makes the Node interface pretty large... Is that really necessary?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:493: static inline bool classof(const NullNode
*) { return true; }
Does the 'inline' keyword actually change anything here?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:512: // utf8).
Why not whatever the original encoding was?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:536: static inline bool classof(const
KeyValueNode *) { return true; }
Is there an underlying strategy how we order the type information vs. the normal
methods?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:541: /// @brief Parse and return the key.
Document that calling this multiple times is ok?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:541: /// @brief Parse and return the key.
I'd document that calling this multiple times is ok...

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:586: if ((Base && Other.Base) &&
Base->CurrentEntry != Other.Base->CurrentEntry)
Just return...

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:613: Type MType;
MappingType? Or call the enum MappingType and this Type?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:702: iterator begin() {
I'm not really happy about the duplication in begin() and skip(), especially as
the change to mid-parse skip() support probably will end up to be exactly the
same... Pull out a base class?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:745: Stream &S;
/me cries. But yea, I know, I'm not in the majority with my strong aversion of
Os.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:756: return S.S.peekNext();
And this is why: I can deduce locally that the first S is the Stream, but I had
to go back and read the Stream implementation to see that the second S is the
Scanner.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:771: void handleTagDirective(const Token &t) {
I assume this is a todo?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:777: bool dir = false;
Style: CapVariables. Also, 'dir' sounds like 'directory'... (yea yea, I know,
I'm strange).

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:809: Token &t = peekNext();
Style: CapVariables

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:846: document_iterator(Document *d) : Doc(d)
{}
Style: CapVariables

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:856: // In place destruct and new are used
here to avoid an unneeded
This looks a little over the top - I don't expect this to be performance
relevant at all...

[Bigcheesegs, 2012-02-05 00:50:24]

On 2012/02/02 10:19:21, klimek wrote:
> First run over the header. Generally, the tooling code doesn't change too much
> with the new interface, so it's definitely in an "I can live with it" state.
> 
> One main high-level design question: Do we really need all the implemented
YAML
> features?

We need the core data model and main syntax, tags, and aliases, yes. The rest of
it, probably not, but they do interact. We haven't fully decided how we plan to
use YAML in the linker yet.

> Some general design things we probably just have different opinions about,
which
> I ran into while changing the tooling code to use the YAMLParser. Feel free to
> strongly object :)
> - I find it easier as a user to have the public section as first section in a
> class definition; you seem to have a mixture, which I find a little confusing

Agreed. I'll fix this.

> - const iterators are missing

I'll add them.

> - I find references to higher-level scope objects as members in a class to be
> bug-prone (you cannot see that the reference is kept from the call site)

I agree, but I'm not sure how to fix this, as the link is needed.

> - I'm a strong opponent of abbrs; I'm not sure whether as a non-native speaker
> it's just harder to read them or I'm just obsessive, but I'm a big fan of
> spending the 0.01 seconds it takes to spell out Current or Position.

I'll fix this where it does not conflict with type names.

I've also responded to the inline comments, but I'm not sure if they get sent
separately.

I'm sending this now without a new patch so that we can discuss some of the
questions I had while I'm fixing some of the larger issues.

Thanks for the review!

- Michael Spencer

[Bigcheesegs, 2012-02-05 05:52:50]

On 2012/02/02 10:19:21, klimek wrote:
> First run over the header. Generally, the tooling code doesn't change too much
> with the new interface, so it's definitely in an "I can live with it" state.
> 
> One main high-level design question: Do we really need all the implemented
YAML
> features?

We need the core data model and main syntax, tags, and aliases, yes. The rest of
it, probably not, but they do interact. We haven't fully decided how we plan to
use YAML in the linker yet.

> Some general design things we probably just have different opinions about,
which
> I ran into while changing the tooling code to use the YAMLParser. Feel free to
> strongly object :)
> - I find it easier as a user to have the public section as first section in a
> class definition; you seem to have a mixture, which I find a little confusing

Agreed. I'll fix this.

> - const iterators are missing

I'll add them.

> - I find references to higher-level scope objects as members in a class to be
> bug-prone (you cannot see that the reference is kept from the call site)

I agree, but I'm not sure how to fix this, as the link is needed.

> - I'm a strong opponent of abbrs; I'm not sure whether as a non-native speaker
> it's just harder to read them or I'm just obsessive, but I'm a big fan of
> spending the 0.01 seconds it takes to spell out Current or Position.

I'll fix this where it does not conflict with type names.

I've also responded to the inline comments, but I'm not sure if they get sent
separately.

I'm sending this now without a new patch so that we can discuss some of the
questions I had while I'm fixing some of the larger issues.

Thanks for the review!

- Michael Spencer

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser.h
File include/llvm/Support/YAMLParser.h (right):

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:18: //    * BOMs anywhere other than the first
Unicode scalar value in the file.
On 2012/02/02 10:19:22, klimek wrote:
> It would be cool to have a pointer to the first class I have to look at as a
> user (Stream). In general, I'm wondering whether we can reduce the size of the
> visible interface here...

Added some user documentation to the top.

I would like to reduce the size. I'm just not sure how best to do it.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:44: UEF_UTF32_LE,
On 2012/02/02 10:19:22, klimek wrote:
> Perhaps document LE vs. BE...

Done.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:64: struct StreamStartInfo {
On 2012/02/02 10:19:22, klimek wrote:
> Is this only used for the scanning? If so, would it make sense to split up the
> header? Why's this a struct instead of a typedef? (do you intend to extend
> this?) Some documentation might help. Same for the next 2 structs.

I do plan to extend a few of these, along with adding a few more (as I currently
overload ScalarInfo for things).

I'll try splitting up the header.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:77: struct Token : ilist_node<Token> {
On 2012/02/02 10:19:22, klimek wrote:
> Are Tokens meant to be changed after being created? If not, I'd vote for a
> non-modifiable design.

They cannot be modified after being created.

By non-modifiable do you mean private members with const getters?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:103: /// A string of length 0 or more who's
begin() points to the logical location
On 2012/02/02 10:19:22, klimek wrote:
> s/who's/whose/

Done.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:108: ///       can't be in a union in C++03.
On 2012/02/02 10:19:22, klimek wrote:
> Why not use inheritance instead?

That would make this a polymorphic class. This should just be a simple value
type.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:155: /// Simple keys are handled by creating
an entry in SimpleKeys for each Token
On 2012/02/02 10:19:22, klimek wrote:
> I don't find the concept of a "simple key" in the YAML spec - I think it would
> be helpful to reference the standard.

They are not mentioned directly in the standard. However, the need for them
comes from grammar rule [151].

I guess I can explain that :P

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:161: /// others) before the SimpleKey's Tok.
On 2012/02/02 10:19:22, klimek wrote:
> In general, this comment doesn't really help me understand the concept. It
> basically explains some coupling, but that looks pretty random when I'm
> contextually at this part of the file.

Agreed, I'll try moving this with the rest of the scanner over to another
header.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:162: struct SimpleKey {
On 2012/02/02 10:19:22, klimek wrote:
> At a first glance this looks like an internal structure used for parsing - can
> we hide this?

It is. I would like to. Not sure how.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:191: if (Pos >= End)
On 2012/02/02 10:19:22, klimek wrote:
> Why can that happen?

There are some places where it errors at the end of a file. For example it
expected something that isn't there. So instead of having clients correct this
before calling, I correct it here.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:215: bool isAtEnd(StringRef::iterator i = 0) {
On 2012/02/02 10:19:22, klimek wrote:
> Unused?

Done.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:233: /// @brief Skip a single nb-char[27]
starting at Pos.
On 2012/02/02 10:19:22, klimek wrote:
> What does 'nb' stand for here? Same for the functions below, I have no idea
what
> those initializations mean.
> 
> Also, here and elsewhere - are those bracketed numbers referring to
> documentation somewhere?
> 
> And lastly, style: shouldn't methods be camelCase?

These are all names taken directly from the yaml spec. Which is also why the
style is different, to closely match the spec.

I have added documentation that explains what the names represent.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:262: /// @brief Skip minimal well-formed code
unit subsequence's until Func
On 2012/02/02 10:19:22, klimek wrote:
> s/'//?

Done.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:270: StringRef::iterator i =
(this->*Func)(Pos);
On 2012/02/02 10:19:22, klimek wrote:
> Style: capital letter for variables?

Not required in this case.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:304: /// @brief Remove simple keys that can no
longer be valid simple keys.
On 2012/02/02 10:19:22, klimek wrote:
> If they can no longer be valid simple keys, perhaps call this
> removeStaleSimpleKeyCandidates?

Done, and also renamed the other functions with SimpleKey in them.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:319: bool rollIndent( int Col
On 2012/02/02 10:19:22, klimek wrote:
> Is the indent here intentional?

Yes.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:413: TokenQueueT TokenQueue;
On 2012/02/02 10:19:22, klimek wrote:
> If this only holds a limited number of tokens (as simple keys are limited to
> 1024 tokens if I understand it correctly), would using a SmallVector lead to
the
> same performance with less code?

The TokenQueue is actually an ilist. This is needed because we must be able to
append and insert Tokens into the queue without invalidating the iterators that
SimpleKeys use to track which tokens they apply to.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:422: class document_iterator;
On 2012/02/02 10:19:22, klimek wrote:
> From your naming I assume that using lower_case for a class is correct style
for
> iterators?

For std:: style code, which would include iterators, yes.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:467: Node(unsigned int Type, Document *D,
StringRef A);
On 2012/02/02 10:19:22, klimek wrote:
> It's really hard to guess what 'A' stands for here without reading the context
-
> can we call it Anchor?

Done.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:477: // These functions forward to Document
and Scanner.
On 2012/02/02 10:19:22, klimek wrote:
> This makes the Node interface pretty large... Is that really necessary?

I don't know how to simplify this.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:493: static inline bool classof(const NullNode
*) { return true; }
On 2012/02/02 10:19:22, klimek wrote:
> Does the 'inline' keyword actually change anything here?

Not sure, this is how the function is defined elsewhere in llvm.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:512: // utf8).
On 2012/02/02 10:19:22, klimek wrote:
> Why not whatever the original encoding was?

Because then the parser would be required to understand these other encodings.
The idea is to convert everything to UTF-8 before parsing. This also simplifies
client code.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:536: static inline bool classof(const
KeyValueNode *) { return true; }
On 2012/02/02 10:19:22, klimek wrote:
> Is there an underlying strategy how we order the type information vs. the
normal
> methods?

After a quick search, it tends to go last in the class. I'll change these.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:541: /// @brief Parse and return the key.
On 2012/02/02 10:19:22, klimek wrote:
> Document that calling this multiple times is ok?

Done.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:541: /// @brief Parse and return the key.
On 2012/02/02 10:19:22, klimek wrote:
> I'd document that calling this multiple times is ok...

Done.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:586: if ((Base && Other.Base) &&
Base->CurrentEntry != Other.Base->CurrentEntry)
On 2012/02/02 10:19:22, klimek wrote:
> Just return...

Done.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:613: Type MType;
On 2012/02/02 10:19:22, klimek wrote:
> MappingType? Or call the enum MappingType and this Type?

Done.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:745: Stream &S;
On 2012/02/02 10:19:22, klimek wrote:
> /me cries. But yea, I know, I'm not in the majority with my strong aversion of
> Os.

Done.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:756: return S.S.peekNext();
On 2012/02/02 10:19:22, klimek wrote:
> And this is why: I can deduce locally that the first S is the Stream, but I
had
> to go back and read the Stream implementation to see that the second S is the
> Scanner.

Done.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:771: void handleTagDirective(const Token &t) {
On 2012/02/02 10:19:22, klimek wrote:
> I assume this is a todo?

Done.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:777: bool dir = false;
On 2012/02/02 10:19:22, klimek wrote:
> Style: CapVariables. Also, 'dir' sounds like 'directory'... (yea yea, I know,
> I'm strange).

Done.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:809: Token &t = peekNext();
On 2012/02/02 10:19:22, klimek wrote:
> Style: CapVariables

Local's don't apply.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:856: // In place destruct and new are used
here to avoid an unneeded
On 2012/02/02 10:19:22, klimek wrote:
> This looks a little over the top - I don't expect this to be performance
> relevant at all...

And it's also totally wrong! :P. Fixed.

[klimek, 2012-02-16 15:52:34]

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser.h
File include/llvm/Support/YAMLParser.h (right):

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:161: /// others) before the SimpleKey's Tok.
On 2012/02/05 05:52:50, Bigcheesegs wrote:
> On 2012/02/02 10:19:22, klimek wrote:
> > In general, this comment doesn't really help me understand the concept. It
> > basically explains some coupling, but that looks pretty random when I'm
> > contextually at this part of the file.
> 
> Agreed, I'll try moving this with the rest of the scanner over to another
> header.

My comment was more about the comment here... I'd change the comment to explain
where simple keys come from.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:191: if (Pos >= End)
On 2012/02/05 05:52:50, Bigcheesegs wrote:
> On 2012/02/02 10:19:22, klimek wrote:
> > Why can that happen?
> 
> There are some places where it errors at the end of a file. For example it
> expected something that isn't there. So instead of having clients correct this
> before calling, I correct it here.

I'm still not sure I understand why:
- Pos > End is possible
- in the case where Pos == End we don't want to point *after* the last character

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:477: // These functions forward to Document
and Scanner.
On 2012/02/05 05:52:50, Bigcheesegs wrote:
> On 2012/02/02 10:19:22, klimek wrote:
> > This makes the Node interface pretty large... Is that really necessary?
> 
> I don't know how to simplify this.

Inline the methods?

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:493: static inline bool classof(const NullNode
*) { return true; }
On 2012/02/05 05:52:50, Bigcheesegs wrote:
> On 2012/02/02 10:19:22, klimek wrote:
> > Does the 'inline' keyword actually change anything here?
> 
> Not sure, this is how the function is defined elsewhere in llvm.

I'd propose to take it out, as no-op code makes people like me wonder whether
there's a reason it's there after all.

http://codereview.appspot.com/5604054/diff/1/include/llvm/Support/YAMLParser....
include/llvm/Support/YAMLParser.h:512: // utf8).
On 2012/02/05 05:52:50, Bigcheesegs wrote:
> On 2012/02/02 10:19:22, klimek wrote:
> > Why not whatever the original encoding was?
> 
> Because then the parser would be required to understand these other encodings.
> The idea is to convert everything to UTF-8 before parsing. This also
simplifies
> client code.

My gut feeling is not yet settled on whether it disagrees, but let's try it your
way first :)

http://codereview.appspot.com/5604054/diff/6001/include/llvm/Support/YAMLPars...
File include/llvm/Support/YAMLParser.h (right):

http://codereview.appspot.com/5604054/diff/6001/include/llvm/Support/YAMLPars...
include/llvm/Support/YAMLParser.h:71: std::string escape(StringRef Input);
This is not called anywhere yet(?) Why do we need it? Are we planning to write
yaml or do we only need to parse it?

http://codereview.appspot.com/5604054/diff/6001/include/llvm/Support/YAMLPars...
include/llvm/Support/YAMLParser.h:90: OwningPtr<Scanner> scanner;
I would strongly argue for consistency and rather see a getScanner() method than
being inconsistent in naming here.

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp
File lib/Support/YAMLParser.cpp (right):

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:667: std::string EscapedInput;
Curious: is there a reason not to use a Twine here (which would be my default
choice for string concatenation).

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:693: EscapedInput += "\\x" +
std::string(HexStr.size() - 2, '0') + HexStr;
Why's the middle part there?

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:694: } else if (*i & 0x80) { // utf8
// utf8
is confusing to me here:
I thought we always assume utf8, and 0x80 merely means that we have a multi-byte
utf8 sequence...

So I'd expect a comment like:
// utf8 multi-byte sequence

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:788: if (   *Position == 0x09
Indentation off?

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:795: if (   u8d.second != 0
Somehow I cannot match this up with 5.1.
Especially:
"""On input, a YAML processor must accept all Unicode characters except those
explicitly excluded above."""

And, why do we care about non-printable chars at all?

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:839: if (  (C >= '0' && C <= '9')
Just return.

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:847: if (  C == '-'
Just return.

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:860: && Current + 2 < End
So we're not inc'ing Current by 3 when encountering %NN because NN will be
parsable anyway?

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:1627: switch (UnquotedValue[0]) {
Do we want to support all possible escapes, or can we get away with the subset
we really need?

[Bigcheesegs, 2012-02-16 22:32:42]

http://codereview.appspot.com/5604054/diff/6001/include/llvm/Support/YAMLPars...
File include/llvm/Support/YAMLParser.h (right):

http://codereview.appspot.com/5604054/diff/6001/include/llvm/Support/YAMLPars...
include/llvm/Support/YAMLParser.h:71: std::string escape(StringRef Input);
On 2012/02/16 15:52:35, klimek wrote:
> This is not called anywhere yet(?) Why do we need it? Are we planning to write
> yaml or do we only need to parse it?

It's used when writing out YAML files.

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp
File lib/Support/YAMLParser.cpp (right):

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:667: std::string EscapedInput;
On 2012/02/16 15:52:35, klimek wrote:
> Curious: is there a reason not to use a Twine here (which would be my default
> choice for string concatenation).

Twine cannot be used here as it is only valid until the end of the statement.
See Twine.h.

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:693: EscapedInput += "\\x" +
std::string(HexStr.size() - 2, '0') + HexStr;
On 2012/02/16 15:52:35, klimek wrote:
> Why's the middle part there?

\x requires two characters after it. This adds the 0 if needed.

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:694: } else if (*i & 0x80) { // utf8
On 2012/02/16 15:52:35, klimek wrote:
> // utf8
> is confusing to me here:
> I thought we always assume utf8, and 0x80 merely means that we have a
multi-byte
> utf8 sequence...
> 
> So I'd expect a comment like:
> // utf8 multi-byte sequence

Done.

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:788: if (   *Position == 0x09
On 2012/02/16 15:52:35, klimek wrote:
> Indentation off?

It lines up with the other side of the || below.

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:795: if (   u8d.second != 0
On 2012/02/16 15:52:35, klimek wrote:
> Somehow I cannot match this up with 5.1.
> Especially:
> """On input, a YAML processor must accept all Unicode characters except those
> explicitly excluded above."""
> 
> And, why do we care about non-printable chars at all?

This excludes the characters listed in 5.1 in addition to the other stuff
removed from nb-char.

This is needed to skip entire UTF-8 minimal well formed subsequences at a time
to keep proper column counts.

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:839: if (  (C >= '0' && C <= '9')
On 2012/02/16 15:52:35, klimek wrote:
> Just return.

Done.

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:847: if (  C == '-'
On 2012/02/16 15:52:35, klimek wrote:
> Just return.

Done.

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:860: && Current + 2 < End
On 2012/02/16 15:52:35, klimek wrote:
> So we're not inc'ing Current by 3 when encountering %NN because NN will be
> parsable anyway?

Yes, however, there's no reason it can't skip all 3.

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:1627: switch (UnquotedValue[0]) {
On 2012/02/16 15:52:35, klimek wrote:
> Do we want to support all possible escapes, or can we get away with the subset
> we really need?

It's only a few extra lines with no extra complexity...

[klimek, 2012-02-21 01:02:57]

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp
File lib/Support/YAMLParser.cpp (right):

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:693: EscapedInput += "\\x" +
std::string(HexStr.size() - 2, '0') + HexStr;
On 2012/02/16 22:32:42, Bigcheesegs wrote:
> On 2012/02/16 15:52:35, klimek wrote:
> > Why's the middle part there?
> 
> \x requires two characters after it. This adds the 0 if needed.

Ok, shouldn't that be 2 - HexStr.size() then?

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:795: if (   u8d.second != 0
On 2012/02/16 22:32:42, Bigcheesegs wrote:
> On 2012/02/16 15:52:35, klimek wrote:
> > Somehow I cannot match this up with 5.1.
> > Especially:
> > """On input, a YAML processor must accept all Unicode characters except
those
> > explicitly excluded above."""
> > 
> > And, why do we care about non-printable chars at all?
> 
> This excludes the characters listed in 5.1 in addition to the other stuff
> removed from nb-char.
> 
> This is needed to skip entire UTF-8 minimal well formed subsequences at a time
> to keep proper column counts.

If I'm not completely misreading, this excludes much more:
"""To ensure readability, YAML streams use only the printable subset of the
Unicode character set. The allowed character range explicitly excludes the C0
control block #x0-#x1F (except for TAB #x9, LF #xA, and CR #xD which are
allowed), DEL #x7F, the C1 control block #x80-#x9F (except for NEL #x85 which is
allowed), the surrogate block #xD800-#xDFFF, #xFFFE, and #xFFFF."""

The surrogate block cannot actually come out of decodeUTF8.

http://codereview.appspot.com/5604054/diff/6001/lib/Support/YAMLParser.cpp#ne...
lib/Support/YAMLParser.cpp:1627: switch (UnquotedValue[0]) {
On 2012/02/16 22:32:42, Bigcheesegs wrote:
> On 2012/02/16 15:52:35, klimek wrote:
> > Do we want to support all possible escapes, or can we get away with the
subset
> > we really need?
> 
> It's only a few extra lines with no extra complexity...

It is code that needs to be maintained ... shrug.