Skip to content

Warning about capture characters#119

Merged
DmitrySoshnikov merged 4 commits into
DmitrySoshnikov:masterfrom
misdocumeno:warn-missing-loc-option
Jun 25, 2022
Merged

Warning about capture characters#119
DmitrySoshnikov merged 4 commits into
DmitrySoshnikov:masterfrom
misdocumeno:warn-missing-loc-option

Conversation

@misdocumeno

Copy link
Copy Markdown
Contributor

If you capture locations in the bnf grammar file used to generate a parser, without using the --loc option, the parser will be successfully generated, but it will throw an error at runtime because the location variables are not defined.

This shows a warning when generating the parser if the grammar file contains location capture characters and the --loc option is not provided.

@DmitrySoshnikov DmitrySoshnikov left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the PR! Some notes inline

Comment thread src/grammar/grammar.js Outdated
Comment thread src/bin/syntax.js Outdated
Comment thread src/grammar/grammar.js
.replace(/"(\\.|[^"\\])*"/g, '')
.replace(/`(\\.|[^`\\])*`/g, '')
// module include
.replace(/%{[\n\s\S]*?%}/g, '');

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Not sure if we need to use all these replace calls, probably just can just do a simple regexp test for:

if (/@\d+/.test(grammar) {
  // Warning
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

without those replaces, the warning would be displayed even when finding @ inside strings, comments, lex rules, etc, which isn't good, for example, css uses that character a lot, for media queries and stuff, so it could be part of a lexer rule if someone tries to make a parser for that language. im sure there are more languages that use that character.
also just using \d in the regex would not show the error when using named tokens (which i do a lot)

let me know if you want me to make that change anyway tho, after all its just a warning, not an error

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yeah, sounds good - the only thing we need to make sure those replaces don't slow down much the handling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i tested it and it doesn't take even 0.015ms in a file of 10 millon characters, at least on my pc. also thats executed only once, and when generating the parser, it won't affect the parser itself at all, so i don't think it's a problem

Comment thread src/bin/syntax.js
@DmitrySoshnikov

Copy link
Copy Markdown
Owner

OK, looks great, thanks!

@DmitrySoshnikov DmitrySoshnikov merged commit 7ba6d0e into DmitrySoshnikov:master Jun 25, 2022
@misdocumeno misdocumeno deleted the warn-missing-loc-option branch June 25, 2022 19:53
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