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

Merge grammar updates to Master for publication #51

Merged
merged 33 commits into from
Feb 26, 2024
Merged

Merge grammar updates to Master for publication #51

merged 33 commits into from
Feb 26, 2024

Conversation

THinnerichs
Copy link
Member

Changes (as far as I remember):

  • Substitute cfgs with csgs entirely (both macros still work)
  • pretty-printing partial programs
  • fix add_rule! failing silently
  • isvariable fix
  • merging grammar
  • grammar expansion should work as intended and shouldn't break every time you call HerbGrammar from HerbBenchmarks

Whebon and others added 24 commits January 18, 2024 10:32
Add a function to merge grammars
This refactor allows for the reuse of the logic that checks
whether a symbol is defined in a specific `Module` or `Main`/`Base`.
Allow the specification one or more `Module` when checking if a
rule used by a `RuleNode` represents a variable, or if it is
defined in one of the specified `Module`s, `Main`, or `Base`.

Fixes #21.
If the expression passed for a rule is malformed, the
function now throws an `ArgumentException`

Fixes #37.
Make `add_rule!(...)` fail audibly
Currently, the rule `true` is equivalent to the rule `1`.
This means that if one of them already exists in the grammar,
the new one is skipped.
@THinnerichs THinnerichs requested a review from ReubenJ February 6, 2024 12:27
@THinnerichs THinnerichs self-assigned this Feb 6, 2024
Copy link
Member

@sebdumancic sebdumancic left a comment

Choose a reason for hiding this comment

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

This all looks fine to me, but just to make sure we make the assumptions explicit: if there is a hole, we simply skip it?

Copy link
Member

@ReubenJ ReubenJ left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just one aesthetic/design comment: I remember reading in an old issue or PR that part of the CFG -> CSG migration would include renaming ContextSensitiveGrammar to Grammar. Did we discuss that option and decide against it? I can see arguments for both keeping the names as they are now, or moving completely to Grammar.

@THinnerichs THinnerichs merged commit 0391326 into master Feb 26, 2024
2 checks passed
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.

6 participants