-
Notifications
You must be signed in to change notification settings - Fork 732
Refactored boxed deconstruct signature code, creating reusable functions #9462
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
Refactored boxed deconstruct signature code, creating reusable functions #9462
Conversation
orizi
left a comment
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.
@orizi made 2 comments.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @eytan-starkware, @ilyalesokhin-starkware, and @TomerStarkware).
crates/cairo-lang-sierra/src/extensions/modules/structure.rs line 284 at r1 (raw file):
impl StructBoxedDeconstructLibfunc { /// Analyzes a struct type to extract member types and snapshot information.
restore doc.
crates/cairo-lang-sierra/src/extensions/modules/utils.rs line 135 at r1 (raw file):
/// Returns the inner type and whether the provided type is a snapshot. pub fn unwrap_snapshot_type(
Suggestion:
pub fn peel_snapshot(
orizi
left a comment
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.
@orizi made 1 comment.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @eytan-starkware, @ilyalesokhin-starkware, and @TomerStarkware).
crates/cairo-lang-sierra/src/extensions/modules/utils.rs line 155 at r1 (raw file):
) -> Result<OutputVarInfo, SpecializationError> { let inner_type = if is_snapshot { snapshot_ty(context, component_ty)? } else { component_ty }; Ok(OutputVarInfo { ty: box_ty(context, inner_type)?, ref_info })
the ref_info part is only confusing.
just handle the typing.
Code quote:
/// Helper to create a boxed output variable for unpack operations.
pub fn boxed_output_var_info(
context: &dyn SignatureSpecializationContext,
component_ty: ConcreteTypeId,
is_snapshot: bool,
ref_info: OutputVarReferenceInfo,
) -> Result<OutputVarInfo, SpecializationError> {
let inner_type = if is_snapshot { snapshot_ty(context, component_ty)? } else { component_ty };
Ok(OutputVarInfo { ty: box_ty(context, inner_type)?, ref_info })
TomerStarkware
left a comment
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.
@TomerStarkware reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @eytan-starkware and @ilyalesokhin-starkware).
f4ea7ec to
36c875a
Compare
eytan-starkware
left a comment
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.
@eytan-starkware made 3 comments.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @orizi).
crates/cairo-lang-sierra/src/extensions/modules/structure.rs line 284 at r1 (raw file):
Previously, orizi wrote…
restore doc.
Done.
crates/cairo-lang-sierra/src/extensions/modules/utils.rs line 155 at r1 (raw file):
Previously, orizi wrote…
the
ref_infopart is only confusing.just handle the typing.
Done.
crates/cairo-lang-sierra/src/extensions/modules/utils.rs line 135 at r1 (raw file):
/// Returns the inner type and whether the provided type is a snapshot. pub fn unwrap_snapshot_type(
Done.
orizi
left a comment
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.
@orizi reviewed all commit messages and made 2 comments.
Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @eytan-starkware, @ilyalesokhin-starkware, and @TomerStarkware).
crates/cairo-lang-sierra/src/extensions/modules/structure.rs line 321 at r2 (raw file):
let mut outputs = Vec::with_capacity(member_types.len()); for member_ty in member_types.by_ref() {
this change broke the current logic.
would probably change tests after rebase as well.
crates/cairo-lang-sierra/src/extensions/modules/utils.rs line 152 at r2 (raw file):
component_ty: ConcreteTypeId, is_snapshot: bool, ) -> Result<ConcreteTypeId, SpecializationError> {
or something better - as the output isn't relevant now.
Suggestion:
/// Helper to create a boxed output variable for unpack operations.
pub fn boxed_ty_with_snapshot(
context: &dyn SignatureSpecializationContext,
component_ty: ConcreteTypeId,
is_snapshot: bool,
) -> Result<ConcreteTypeId, SpecializationError> {612b112 to
a0f42e0
Compare
eytan-starkware
left a comment
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.
@eytan-starkware made 2 comments.
Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @ilyalesokhin-starkware, @orizi, and @TomerStarkware).
crates/cairo-lang-sierra/src/extensions/modules/structure.rs line 321 at r2 (raw file):
Previously, orizi wrote…
this change broke the current logic.
would probably change tests after rebase as well.
f2f
crates/cairo-lang-sierra/src/extensions/modules/utils.rs line 152 at r2 (raw file):
Previously, orizi wrote…
or something better - as the
outputisn't relevant now.
Done.
a0f42e0 to
4e956b9
Compare
orizi
left a comment
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.
@orizi reviewed 2 files and all commit messages, made 1 comment, and resolved 5 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @eytan-starkware and @ilyalesokhin-starkware).
crates/cairo-lang-sierra/src/extensions/modules/utils.rs line 145 at r3 (raw file):
Ok((ty, false)) } }
Suggestion:
pub fn peel_snapshot(
context: &dyn SignatureSpecializationContext,
ty: &ConcreteTypeId,
) -> Result<(&ConcreteTypeId, bool), SpecializationError> {
let type_info = context.get_type_info(ty)?;
if type_info.long_id.generic_id == SnapshotType::id() {
Ok((args_as_single_type(&type_info.long_id.generic_args)?, true))
} else {
Ok((ty, false))
}
}4e956b9 to
7b6d2af
Compare
9f17c72 to
bf6b4e1
Compare
eytan-starkware
left a comment
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.
@eytan-starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @orizi).
crates/cairo-lang-sierra/src/extensions/modules/utils.rs line 145 at r3 (raw file):
Ok((ty, false)) } }
Done
7b6d2af to
a793aa3
Compare
bf6b4e1 to
44b56da
Compare
orizi
left a comment
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.
@orizi reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware).
44b56da to
2f97046
Compare
a793aa3 to
2ba5194
Compare
2f97046 to
f07d02a
Compare
2ba5194 to
0e998a4
Compare
f07d02a to
9825250
Compare
0e998a4 to
442b9c0
Compare
Merge activity
|
SIERRA_UPDATE_MINOR_CHANGE_TAG=Refactored boaxed deconstruct to be more reusable
442b9c0 to
de22370
Compare
orizi
left a comment
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.
@orizi reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware).

Summary
Refactored the struct boxed deconstruct libfunc to improve handling of output variable references. This PR:
Extracted common utility functions to a new
utils.rsmodule:unwrap_snapshot_typeto handle snapshot type detection and unwrappingboxed_output_var_infoto create boxed output variables consistentlyImproved the struct deconstruction logic:
Type of change
Please check one:
Why is this change needed?
The previous implementation of struct boxed deconstruction had complex logic for handling output variable references, especially when dealing with zero-sized types. This refactoring makes the code more maintainable and consistent by extracting common patterns into utility functions and improving the handling of output variable references.
What was the behavior or documentation before?
The struct boxed deconstruct libfunc had complex inline logic for handling snapshots and determining output variable references. The code was less maintainable and had a more complex approach to handling zero-sized types.
What is the behavior or documentation after?
The functionality remains the same, but the code is now more maintainable with:
Additional context
This refactoring is part of ongoing efforts to improve the maintainability of the Sierra codebase, particularly around struct operations and boxed types.