Parser syntax sparse bl #76
Conversation
zbraniecki
force-pushed
the
parser-syntax-sparse-bl
branch
from
9ce7bf8 to
188d717
Compare
|
I think this is ready to be reviewed. I analyzed the change in span ranges and the cause is that we now start the span for message value after we skip the inline whitespace. For example: and: |
|
I'm still testing performance but first indications are that it doesn't impact runtime perf, just the fluent-syntax parsing one, which is not significant for us. I tested on node, need to recompile spidermonkey to test against it. |
|
Ok, got some more performance tests. This patch does actually regress on jsshell: I have no idea why the |
|
Here's the same data for nodejs: So, still a regression but not as significant as on jsshell. I have no idea what's up with jsshell... maybe with the patch we cross some boundary due to, I don't know, number of variables? Lines of code? |
fluent-syntax/src/ftlstream.js
Outdated
| @@ -31,6 +31,21 @@ export class FTLParserStream extends ParserStream { | |||
| } | |||
| } | |||
|
|
|||
| peekSkipBlankLines() { | |||
There was a problem hiding this comment.
This name is confusing. Does it peek or does it skip?
fluent-syntax/src/parser.js
Outdated
| @@ -242,6 +243,7 @@ export default class FluentParser { | |||
|
|
|||
| while (true) { | |||
| ps.expectChar('\n'); | |||
| ps.skipBlankLines(); | |||
| ps.skipInlineWS(); | |||
There was a problem hiding this comment.
This looks like a common pattern. Perhaps it's worth having a function called expectIndent?
There was a problem hiding this comment.
I was intrigued by the fact that neither getTags nor getAttributes didn't need skipBlankLines. It turns out that they currently use skipWS which is too tolerant about indentation. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1405645.
fluent-syntax/src/stream.js
Outdated
| resetPeek() { | ||
| this.peekIndex = this.index; | ||
| this.peekEnd = this.iterEnd; | ||
| resetPeek(pos = false) { |
There was a problem hiding this comment.
Nit: pos doesn't require a default: it will be undefined if nothing is passed.
fluent/src/parser.js
Outdated
| @@ -344,6 +367,16 @@ class RuntimeParser { | |||
| // by new line and `|` character at the beginning of the next one. | |||
There was a problem hiding this comment.
Would you mind fixing this comment while you're at it, please?
|
I filed bug 1406880 because it looks like the spans of multiline Patterns are slightly different from the ones visualized in your comment above. I'm not yet sure which approach is better, let's decide in the bug and fix this if needed. |
- Implement Fluent Syntax 0.5.
- Add support for terms.
- Add support for `#`, `##` and `###` comments.
- Remove support for tags.
- Add support for `=` after the identifier in message and term
defintions.
- Forbid newlines in string expressions.
- Allow trailing comma in call expression argument lists.
In fluent-syntax 0.6.x the new Syntax 0.5 is supported alongside the old
Syntax 0.4. This should make migrations easier.
`FluentParser` will correctly parse Syntax 0.4 comments (prefixed with
`//`), sections and message definitions without the `=` after the
identifier. The one exception are tags which are no longer supported.
Please use attributed defined on terms instead.
`FluentSerializer` always serializes using the new Syntax 0.5.
- Add `AST.Placeable` (#64)
Added in Syntax Spec 0.4, `AST.Placeable` provides exact span data about
the opening and closing brace of placeables.
- Expose `FluentSerializer.serializeExpression`. (#134)
- Serialize standalone comments with surrounding white-space.
- Allow blank lines inside of messages. (#76)
- Trim trailing newline from Comments. (#77)
That's more or less how I think it'll have to work if we want to keep the spans and proper tokenizer flow.
I can't say I like it... :(