-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add HashableValue as part of ConstValue, and ContainerValue #322
Conversation
} | ||
} | ||
|
||
#[cfg(test)] |
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.
These have gone to ops/constant.rs. I could write some HashableValue-specific ones in values.rs
...
@@ -87,132 +54,3 @@ pub(crate) fn check_int_fits_in_width( | |||
Err(ConstIntError::IntWidthInvalid(width)) | |||
} | |||
} | |||
|
|||
fn map_vals<T: PrimType, T2: PrimType>( |
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.
This has moved to values.rs
} | ||
|
||
/// Typecheck a constant value | ||
pub(super) fn typecheck_const(typ: &ClassicType, val: &ConstValue) -> Result<(), ConstTypeError> { |
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.
This has been split into values.rs and constant.rs parts
/// An arbitrary length integer constant. | ||
Int(HugrIntValueStore), | ||
Hashable(HashableValue), | ||
/// A collection of constant values (at least some of which are not [ConstValue::Hashable]) |
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.
"not known to be", perhaps
src/ops/constant.rs
Outdated
match ty { | ||
ClassicType::Container(cty) => return vals.check_container(cty), | ||
// We might also fail to deduce a container *value* was hashable, | ||
// because it contains opaque values which don't synthesize their type. |
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.
This comment might need rewording given #325 provides no way for a value "to synthesize its type". Perhaps it motivates allowing such a way?
src/ops/constant.rs
Outdated
@@ -145,57 +149,128 @@ impl PartialEq for dyn CustomConst { | |||
|
|||
impl Default for ConstValue { | |||
fn default() -> Self { | |||
Self::Int(0) | |||
Self::Hashable(HashableValue::Int(0)) |
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.
i think a default no longer makes sense
src/values.rs
Outdated
} | ||
ContainerValue::Map(_) => "a map".to_string(), | ||
ContainerValue::Sum(tag, val) => format!("const:sum:{{tag:{tag}, val:{}}}", val.name()), | ||
ContainerValue::Opaque(c) => format!("const:{:?}", c), |
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.
worth saying opaque here?
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.
Used const:custom:
and const:yaml:
for the two different forms that we still have ATM
src/values.rs
Outdated
/*/// Expected width (packed with const int) doesn't match type | ||
#[error("Type mismatch for int: expected I{0}, but found I{1}")] | ||
IntWidthMismatch(HugrIntWidthStore, HugrIntWidthStore),*/ |
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.
delete
Add src/values.rs w/new {Hashable,Container}Value, also move ConstTypeError etc. +trait ValueOfType defines 'check_type' method breaking up old typecheck_const src/ops/constant.rs contains ConstValue (which uses HashableValue) src/ops/constant/typecheck.rs contains bits still shared with TypeArg/TypeParam
serde_yaml::Value
andBox<dyn CustomConst>
in different placestypecheck.rs
remains, as the code that's used by both the new and the old, not-yet-updated (TypeParam/TypeArg) code. When we update the latter, the classes/routines intypecheck.rs
should only have one caller (in values.rs) and then they can be moved into values.rs. [So, this is kinda in the wrong place now, if we want to move it.]