Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add example grammars for XML and source-mapping:mapping #196

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

hildjj
Copy link
Contributor

@hildjj hildjj commented Oct 16, 2021

To understand source-maps better, I wrote a grammar for the "mapping" field a few weeks ago. In the discussion about #194, I realized we didn't have an XML grammar, so I whipped one up from the spec text.

const loc = location()
loc.start.offset -= n.length;
loc.start.column -= n.length;
error(`Expected end tag "${other}" but got "${n}"`, loc)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only interesting part of the grammar, other than figuring out what the XML spec means exactly by -.

}

source_fields = source:field source_line:field source_col:field name:field? {
const ret = { source, source_line, source_col }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you add this field then the final object will enumerate properties in the same order as they is defined in the parsed string

Suggested change
const ret = { source, source_line, source_col }
const ret = { gen_col: 0, source, source_line, source_col }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.


// Character Data

CharData = value:$(!']]>' [^<&])* { return value ? {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it is better to use named rule unless this grammar is not a relatively straightforward rewrite of the grammar from the standard.

Suggested change
CharData = value:$(!']]>' [^<&])* { return value ? {
CharData = value:$(!CDEnd [^<&])* { return value ? {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, and replaced all other ']]>' as well.

Comment on lines 32 to 44
function convertAttr(attr, error) {
const ret = {}
for (const {name, value, loc} of attr) {
if (ret[name]) {
error(`Duplicate attribute "${name}"`, loc)
}
ret[name] = value
}
return ret
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you always supply the same argument to the 2nd parameter it is better just to declare this function in the per-parse initializer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

loc.start.offset -= n.length;
loc.start.column -= n.length;
error(`Expected end tag "${other}" but got "${n}"`, loc)
} { return n }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use plucking together with semantic predicates, maybe we should show that capability here:

// Returns result of the `Name` if `&{...}` matched
popName = @n:Name &{ ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this. Done.

@hildjj
Copy link
Contributor Author

hildjj commented Oct 18, 2021

@Mingun let me know when you're done and I'll push changes.

Copy link
Member

@Mingun Mingun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you should add some comments to source-mappings.peggy that explains some tricks with bits. In any case, LGTM

@hildjj
Copy link
Contributor Author

hildjj commented Oct 18, 2021

Added some comments and did a slight rework of source-mappings to make it a little clearer.

@hildjj hildjj changed the base branch from main to 1.3 October 18, 2021 20:09
@hildjj hildjj merged commit 04eb7b0 into peggyjs:1.3 Oct 18, 2021
@hildjj hildjj deleted the more-examples branch October 18, 2021 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants