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

AST post-processing #116

Closed
hamishknight opened this issue Jan 18, 2022 · 8 comments · Fixed by #379
Closed

AST post-processing #116

hamishknight opened this issue Jan 18, 2022 · 8 comments · Fixed by #379

Comments

@hamishknight
Copy link
Contributor

This issue tracks logic that needs to be implemented after the parser has produced an AST.

To be implemented:

  • Group reference validity checking (as group references may refer to groups that come after them)
    • This will require deciding how to handle the ambiguity with the syntax (?(xxx)). PCRE always treats this as a named reference. .NET only treats it as a named reference if there is a group defined with that name, otherwise it treats it as an arbitrary regex condition. It's possible we may want to require users explicitly spell named references (?('xxx')) to avoid this ambiguity.
  • Errors for AST nodes that are unsupported by the matching engine
  • Warnings for syntax that has a better spelling
@milseman
Copy link
Member

Is there global data we might also want to store?

@hamishknight
Copy link
Contributor Author

hamishknight commented Jan 20, 2022

@milseman Did you have something specific in mind? Not sure if this is what you mean, but we will need to decide where to store the global matching options that PCRE allows to be parsed at the start of the regex, e.g (*CR) & (*UTF). I have a patch that parses them, and for now stores them as an AST node that we'll produce if they're there. But it's possible we may want to have the parser produce an "AST root" type that wraps an AST along with the global options. I don't really have a good sense of what will be more convenient for consumers of the AST though. I don't know if you've had any thoughts on that?

@milseman
Copy link
Member

It may make sense to have struct AST and enum AST.Node. So we have a struct that has an enum that stores structs...

@hamishknight
Copy link
Contributor Author

@milseman Makes sense, presumably recursive AST nodes would then store AST.Node instead of AST?

@hamishknight
Copy link
Contributor Author

This is mostly handled by #379, I've split off #380 for the parser warnings.

@milseman
Copy link
Member

milseman commented May 6, 2022

@hamishknight should this be closed then or is it tracking something?

@hamishknight
Copy link
Contributor Author

I was going to close it with the merging of #379, but am also happy to close now

@milseman
Copy link
Member

milseman commented May 6, 2022

Oh right, forgot Github had that feature. Neat.

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 a pull request may close this issue.

2 participants