-
Notifications
You must be signed in to change notification settings - Fork 51
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
Expose TermID #207
Expose TermID #207
Conversation
Follow up from egraphs-good#202 to expose the TermID so that we can create/access them in the Python bindings.
Whoops, good catch. |
If we need to expose the innards of TermId for python, then there's no point in having that type at all, and we should just go back to usizes. Sorry, I didn't really understand the constraints of they python integration when I refactored terms like this. |
The current Python integration is basically data in, data out. Every struct/enum which is passed into or returned from any of the needed egraph methods are exposed as Python classes (implemented as new Rust structs). The only object which shares a reference between Python and Rust is the EGraph itself, everything else is copied. So if we want to expose any struct methods to Python, we would have to convert the struct and the arguments first, then call it, and then convert the return value. Of course, we could change any of that, but as a first pass in getting it up to date with the new changes, I was trying to do the minimal needed. |
Ok makes sense. In that case I think we should change TermIds to not have a wrapper struct, since the only reason I put the wrapper struct there was to make the field not public. So it should be type TermId = usize; and then a couple fixes throughout that file as a result (removing wrap/unwraps). Let me know if you want me to do this for you. |
Sorry we broke this @saulshanabrook! |
As per suggestion from @wilcoxjay egraphs-good#207 (comment)
LGTM! |
Follow up from #202 to expose the TermID so that we can create/access them in the Python bindings.
See #192 for the original PR which made these public.