Skip to content

Conversation

alex-ozdemir
Copy link
Member

We want sort-specific operator overloading, but Python operator
overloading is based on classes. Thus, we create
(a) class hierarchies which match the Sort hierarchy
(b) functions to construct the correct class based on the sort

Expr hierarchy also has elements for concrete values.
This will be useful for interacting with models.

Actual overloading is still a ways off...

We want sort-specific operator overloading, but Python operator
overloading is based on classes. Thus, we create
  (a) class hierarchies which match the Sort hierarchy
  (b) functions to construct the correct class based on the sort

Expr hierarchy also has elements for concrete values.
This will be useful for interacting with models.

Actual overloading is still a ways off...
@alex-ozdemir alex-ozdemir requested a review from makaimann May 19, 2021 21:44
Copy link
Member

@makaimann makaimann left a comment

Choose a reason for hiding this comment

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

It looks pretty good to me, I left a few general comments. One higher-level question is about the class hierarchy: is that from the Z3 API? It makes sense, although you could also do a case split within the comparison operator. I suppose that might be less efficient.

I'm wondering when would you do the cast to the specific sort class? Will all sort objects that a user interacts with be an instance of one of the specific classes?

I'm interested in the rationale, although if this is just to best match the API, then I recognize that we don't really have a choice.

True
>>> is_sort(Int('x'))
False
>>> is_expr(Int('x'))
Copy link
Member

Choose a reason for hiding this comment

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

Do you have an is_expr? Maybe remove that from the example if not.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have it yet, but we will.

My instinct is to leave this here for now; that will ensure that we're reminded of this when we enable doc-testing. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

@alex-ozdemir
Copy link
Member Author

It looks pretty good to me, I left a few general comments. One higher-level question is about the class hierarchy: is that from the Z3 API? It makes sense, although you could also do a case split within the comparison operator. I suppose that might be less efficient.

Yeah, this class hierarchy is from the Z3 API, so we're stuck with it.

I'm not sure about the comparative efficiency of this approach v. a case-split. Perhaps Leo thought this approach would produce better error messages (e.g., a boolean expression doesn't have a "+" operation). It might make for nicer code, too. While I generally prefer the case-split (FP) approach to the sub-class (OO) approach, the latter is the strongest when you want extensibility to new kids of data, which is our situation here.

I'm wondering when would you do the cast to the specific sort class? Will all sort objects that a user interacts with be an instance of one of the specific classes?

Anything given the user will be run through _to_expr_ref (or _to_sort_ref), so yep!

Copy link
Member

@makaimann makaimann left a comment

Choose a reason for hiding this comment

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

Cool, thanks for your responses! It looks good to me.

@alex-ozdemir alex-ozdemir merged commit b31907b into main May 25, 2021
@alex-ozdemir alex-ozdemir deleted the hierarchy branch May 25, 2021 22: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