Serialize let bindings#701
Conversation
Closes egraphs-good#376 by storing the let bindings on the serialized format. This also will show them in the visualizer now
CodSpeed Performance ReportMerging #701 will not alter performanceComparing Summary
Footnotes
|
|
This is now ready for review now that egraph-serialize has a new version. |
…iption as metadata
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the ignore_viz field with let_binding to properly serialize let bindings in the egraph output format, enabling them to be displayed in the visualizer.
Key Changes:
- Renamed
ignore_viztolet_bindingthroughout the codebase to better reflect its semantic purpose - Modified serialization logic to store let bindings as metadata on e-classes rather than filtering them out
- Updated
egraph-serializedependency from git branch to version 0.3
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/ast/mod.rs | Renamed ignore_viz field to let_binding in FunctionDecl struct and its constructors |
| src/ast/remove_globals.rs | Updated field name from ignore_viz to let_binding for global removal logic |
| src/serialize.rs | Refactored serialization to collect and store let bindings in class metadata instead of filtering them |
| src/typechecking.rs | Updated field reference from ignore_viz to let_binding |
| Cargo.toml | Changed egraph-serialize from git dependency to version 0.3 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@codex review |
yihozhang
left a comment
There was a problem hiding this comment.
Missed this PR, sorry!
This looks like a breaking change because the public field ignore_viz is renamed. Maybe we should find a way to track when a breaking change is introduced?
I added an entry to the changelog that makes it explicit it's breaking. |
Closes #376 by storing the let bindings on the serialized format.
This also will show them in the visualizer now:
Please review and merge egraphs-good/egraph-serialize#23 first and then I can update the dep here to pull from main.