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

Calculate used variable names better in buildRule #2698

Merged
merged 15 commits into from
Jul 5, 2022
Merged

Conversation

ehildenb
Copy link
Member

@ehildenb ehildenb commented Jul 3, 2022

This PR does several things:

  • Moves splitConfigAndConstraint to inside CTerm, so now there are no dependencies of cterm.py on kastManip.py.
  • Renames variables in buildRule, and renames method to build_rule.
  • Lifts build_rule to operate over CTerm, instead of over KInner.
  • Adds a test where build_rule is producing variable names in the rule which cause the frontend to emit warnings.
  • Fixes bug where build_rule is naming variables poorly.
  • Adds a method with_constraint to CTerm for easily making new CTerm with an added constraint.
  • Removes an old test of build_rule that becomes redundant with the added tests here.
  • Moves the test_configuration.py tests into unit tests under test_kastManip.py so they don't build definitions every time (they don't need to).

The main change to buildRule is that if you have a rule:

    rule <k> K_CELL ... </k> <mem> MEM => MEM' </mem>

It will now realize that K_CELL should be anonymous, and rename it to _K_CELL before feeding it to the frontend. This is to avoid a bunch of frontend warnings.

@ehildenb ehildenb self-assigned this Jul 3, 2022
@ehildenb ehildenb marked this pull request as ready for review July 4, 2022 03:32
@ehildenb ehildenb requested review from nishantjr and tothtamas28 July 4, 2022 03:33
Comment on lines 96 to 97
def with_constraint(self, new_constraint: KInner) -> 'CTerm':
return CTerm(mlAnd([self.config, new_constraint] + list(self.constraints), Sorts.GENERATED_TOP_CELL))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd call it add_constraint instead. To me, with_constraint suggests "take the configuration, and couple it with the constraint provided."

Suggested change
def with_constraint(self, new_constraint: KInner) -> 'CTerm':
return CTerm(mlAnd([self.config, new_constraint] + list(self.constraints), Sorts.GENERATED_TOP_CELL))
def add_constraint(self, constraint: KInner) -> 'CTerm':
return CTerm(mlAnd((self.config, constraint) + self.constraints, Sorts.GENERATED_TOP_CELL))

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

constraints = CTerm._normalize_constraints(flattenLabel('#And', constraint))
object.__setattr__(self, 'config', config)
object.__setattr__(self, 'constraints', constraints)

@staticmethod
def _split_config_and_constraints(kast: KInner) -> Tuple[KInner, KInner]:
Copy link
Contributor

Choose a reason for hiding this comment

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

As a first step, I'd just move this to cterm.py. Later, when we are ready to eliminate calls to the function, e.g. by replacing

config, constraint = split_config_and_constraints(term)

by

config, *constraints = Cterm(term)
constraint = mlAnd(constraints)

we can make it a private static method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@nishantjr nishantjr 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.

@rv-jenkins rv-jenkins merged commit 2b478c0 into master Jul 5, 2022
@rv-jenkins rv-jenkins deleted the rule-variables branch July 5, 2022 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants