-
Notifications
You must be signed in to change notification settings - Fork 32
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
Remove unused code from starkware_utils #440
Conversation
@@ -174,20 +174,7 @@ where | |||
ffc: &'a mut FactFetchingContext<S, H>, | |||
indices: &'a [TreeIndex], | |||
facts: &'a mut Option<BinaryFactDict>, | |||
) -> impl std::future::Future<Output = Result<HashMap<TreeIndex, LF>, TreeError>> + Send { |
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.
Any reason why you have removed this default implementation?
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.
No particular reason except it's never used. This trait is only implemented in virtual_patricia_node.rs
, and it has it's own implementation of _get_leaves.
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.
Left a single comment but most seems good.
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 looks good 💪
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.
A lot of what is removed here was from the original port of the pythonic impl to rust. That was done in as much of a literal fashion as was possible, and that's probably why there is a lot of dead code.
I can see arguments for and against keeping this code, so perhaps we should discuss internally before we decide.
For example, you can grep for a lot of the fn and struct names here in cairo-lang
and see that they exist there.
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!
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
Issue Number: N/A
Type
Description
Removed several unused methods and unreachable code from
starkware_utils
.Breaking changes?