Skip to content
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

fix rewrites of ordered_hash_map so the key is also rewritten #6945

Merged
merged 1 commit into from
Dec 29, 2024

Conversation

TomerStarkware
Copy link
Collaborator

No description provided.

@TomerStarkware TomerStarkware requested a review from orizi December 28, 2024 02:09
@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @TomerStarkware)


crates/cairo-lang-semantic/src/expr/inference.rs line 387 at r1 (raw file):

                .type_assignment
                .iter()
                .map(|(k, v)| (*k, inference_id_replacer.rewrite(*v).no_err()))

this sort of rewrite sounds like the same thing.

Code quote:

            type_assignment: self
                .type_assignment
                .iter()
                .map(|(k, v)| (*k, inference_id_replacer.rewrite(*v).no_err()))

crates/cairo-lang-semantic/src/substitution.rs line 213 at r1 (raw file):

        *value = temp_value
            .into_iter()
            .map(|(mut k, mut v)| {

This makes map rewrite much heavier - as it actually has to create a new map.

this wasn't this way on purpose, we need to either makes sure this doesn't hurt performance (note the explicit calling for this sort of rewrites in other areas, like the inference rewrite), or just call this heavy version when required.

Code quote:

        let mut result = RewriteResult::NoChange;
        let temp_value = std::mem::take(value);
        *value = temp_value
            .into_iter()
            .map(|(mut k, mut v)| {

@TomerStarkware TomerStarkware force-pushed the tomer/fix_rewrite_of_orderedhashmap branch from 225dab9 to 777f52b Compare December 29, 2024 10:00
Copy link
Collaborator Author

@TomerStarkware TomerStarkware left a 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)


crates/cairo-lang-semantic/src/substitution.rs line 213 at r1 (raw file):

Previously, orizi wrote…

This makes map rewrite much heavier - as it actually has to create a new map.

this wasn't this way on purpose, we need to either makes sure this doesn't hurt performance (note the explicit calling for this sort of rewrites in other areas, like the inference rewrite), or just call this heavy version when required.

Now I am only cloning the keys when there is no rewrite,
also we only use this method with copiable keys so we can change T to be copiable


crates/cairo-lang-semantic/src/expr/inference.rs line 387 at r1 (raw file):

Previously, orizi wrote…

this sort of rewrite sounds like the same thing.

here the key is not rewritable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)


crates/cairo-lang-semantic/src/substitution.rs line 213 at r1 (raw file):

Previously, TomerStarkware wrote…

Now I am only cloning the keys when there is no rewrite,
also we only use this method with copiable keys so we can change T to be copiable

do note that you should know this from the type of the keys (if they have 'identity-rewrite') - but maybe that's enough.

@TomerStarkware TomerStarkware force-pushed the tomer/fix_rewrite_of_orderedhashmap branch from 4b558ba to 04a899c Compare December 29, 2024 13:52
@TomerStarkware TomerStarkware force-pushed the tomer/fix_rewrite_of_orderedhashmap branch from 04a899c to ac6bde3 Compare December 29, 2024 13:52
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TomerStarkware)

@TomerStarkware TomerStarkware added this pull request to the merge queue Dec 29, 2024
Merged via the queue into main with commit e133839 Dec 29, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants