-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: Constant values in hugr-model
#1838
Conversation
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary
|
2bc0a18
to
565e18a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1838 +/- ##
==========================================
- Coverage 86.67% 86.55% -0.13%
==========================================
Files 194 194
Lines 35010 35156 +146
Branches 31823 31969 +146
==========================================
+ Hits 30346 30430 +84
- Misses 2939 2968 +29
- Partials 1725 1758 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, biggest question is about ->
placement
node_to_id: FxHashMap<Node, model::NodeId>, | ||
|
||
/// Mapping from node ids in the [`Hugr`] to the corresponding model nodes. | ||
id_to_node: FxHashMap<model::NodeId, Node>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if these are meant to stay in sync would be better to have a dedicated struct holding both that has responsibility for their correctness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally yes. I'm not entirely sure yet if the way that functions are exported is ideal here, so there might be a bit of churn around these things happening. In that light I'm not sure how much it is worth to polish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leave a todo perhaps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo CI fixes 👍
4cc2572
to
3a50604
Compare
Import and export constant values via
hugr-model
.Includes terms corresponding to
Value::Function
and allows exporting; importing these is left for a later PR since sum/extension constants are the most important for now. In the future, we should move from the JSON/typetag based model for extension constants to one based on custom constructors.