-
Notifications
You must be signed in to change notification settings - Fork 686
Fix handling of const_value in extract_generic_params. #8797
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
base: main
Are you sure you want to change the base?
Conversation
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.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)
crates/cairo-lang-semantic/src/types.rs line 242 at r1 (raw file):
TypeLongId::FixedSizeArray { type_id, size } => { type_id.long(db).extract_generic_params(db, generic_parameters)?; size.ty(db)?.long(db).extract_generic_params(db, generic_parameters)?;
size value may also be generic param
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.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @TomerStarkware)
crates/cairo-lang-semantic/src/types.rs line 242 at r1 (raw file):
TypeLongId::FixedSizeArray { type_id, size } => { type_id.long(db).extract_generic_params(db, generic_parameters)?; size.ty(db)?.long(db).extract_generic_params(db, generic_parameters)?;
size.ty(db)? is always coerced to be usize. - i don't think this line in needed.
Code quote:
size.ty(db)?.long(db).extract_generic_params(db, generic_parameters)?;444f742 to
21a131d
Compare
62a7ea1 to
ad6bc05
Compare
ilyalesokhin-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.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @orizi and @TomerStarkware)
crates/cairo-lang-semantic/src/types.rs line 242 at r1 (raw file):
Previously, TomerStarkware wrote…
size value may also be generic param
Done.
crates/cairo-lang-semantic/src/types.rs line 242 at r1 (raw file):
Previously, orizi wrote…
size.ty(db)? is always coerced to be
usize. - i don't think this line in needed.
I think it ok to make it handle the generic case here.
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 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @TomerStarkware)
crates/cairo-lang-semantic/src/items/constant.rs line 160 at r3 (raw file):
ConstValue::Enum(concrete_variant, _const_value_id) => { concrete_variant.ty.long(db).extract_generic_params(db, generic_parameters)? }
Suggestion:
ConstValue::Enum(concrete_variant, const_value_id) => {
concrete_variant.ty.long(db).extract_generic_params(db, generic_parameters)?;
const_value_id.extract_generic_params(db, generic_parameters)?
}ad6bc05 to
3f81c3f
Compare
ilyalesokhin-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.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @orizi and @TomerStarkware)
crates/cairo-lang-semantic/src/items/constant.rs line 160 at r3 (raw file):
ConstValue::Enum(concrete_variant, _const_value_id) => { concrete_variant.ty.long(db).extract_generic_params(db, generic_parameters)? }
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 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)
ilyalesokhin-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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @orizi and @TomerStarkware)
crates/cairo-lang-semantic/src/items/constant.rs line 165 at r2 (raw file):
} ConstValue::Generic(generic_param_id) => { generic_parameters.insert(*generic_param_id);
If I do this I get a crash here:
https://github.com//starkware-libs/cairo/blob/eb9f7309af3bd815aca96187af06b4a2f662a089/crates/cairo-lang-semantic/src/substitution.rs#L543
Code quote:
generic_parameters.insert(*generic_param_id);
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @TomerStarkware)
crates/cairo-lang-semantic/src/items/constant.rs line 165 at r2 (raw file):
Previously, ilyalesokhin-starkware wrote…
If I do this I get a crash here:
https://github.com//starkware-libs/cairo/blob/eb9f7309af3bd815aca96187af06b4a2f662a089/crates/cairo-lang-semantic/src/substitution.rs#L543
what exactly?
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 all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @orizi)
crates/cairo-lang-semantic/src/items/constant.rs line 165 at r2 (raw file):
Previously, orizi wrote…
what exactly?
You are creating a type var for this constant parameter; it needs a const var
No description provided.