Skip to content

refactor(starknet): remove redundant clone calls in dispatcher#9692

Open
radik878 wants to merge 2 commits intostarkware-libs:mainfrom
radik878:refactor/dispatcher-remove-redundant-clones
Open

refactor(starknet): remove redundant clone calls in dispatcher#9692
radik878 wants to merge 2 commits intostarkware-libs:mainfrom
radik878:refactor/dispatcher-remove-redundant-clones

Conversation

@radik878
Copy link
Copy Markdown
Contributor

Summary

Changed ret_decode parameter from String to &str and updated call sites to pass references instead of cloned values, eliminating 7 unnecessary allocations per interface function processed.


Type of change

  • Style, wording, formatting, or typo-only change

Why is this change needed?

The dispatcher plugin was making unnecessary .clone() calls when passing ret_decode (String), serialization_code (Vec), and entry_point_selector (RewriteNode) to the declaration_method_impl function. These values were either moved or not modified after being passed, making the clones redundant.


@chatgpt-codex-connector
Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@reviewable-StarkWare
Copy link
Copy Markdown

This change is Reviewable

Copy link
Copy Markdown
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:

@orizi reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on radik878).

Copy link
Copy Markdown
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.

@orizi made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on radik878).


crates/cairo-lang-starknet/src/plugin/dispatcher.rs line 435 at r1 (raw file):

        Result::Ok(\n        $deserialization_code$\n        )"
            )
        },

this actually has possibly more clones than before still.

Suggestion:

    serialization_code: Vec<RewriteNode<'db>>,
    ret_decode: String,
    unwrap: bool,
) -> RewriteNode<'db> {
    let ret_decode_is_empty = ret_decode.is_empty();
    let deserialization_code = if ret_decode_is_empty {
        RewriteNode::text("()")
    } else {
        RewriteNode::Text(if unwrap {
            ret_decode
        } else {
            ret_decode.split('\n').map(|x| format!("    {x}")).join("\n")
        })
    };
    let return_code = RewriteNode::interpolate_patched(
        &if unwrap {
            format!(
                "let mut {RET_DATA} = starknet::SyscallResultTrait::unwrap_syscall({RET_DATA});
        $deserialization_code$"
            )
        } else if ret_decode_is_empty {
            format!(
                "let mut {RET_DATA} = {RET_DATA}?;
        Result::Ok($deserialization_code$)"
            )
        } else {
            format!(
                "let mut {RET_DATA} = {RET_DATA}?;
        Result::Ok(\n        $deserialization_code$\n        )"
            )
        },

@radik878
Copy link
Copy Markdown
Contributor Author

radik878 commented Feb 18, 2026

    ret_decode: String

ret_decode: String didn't compile

Copy link
Copy Markdown
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.

@orizi reviewed all commit messages and made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on radik878).


crates/cairo-lang-starknet/src/plugin/dispatcher.rs line 435 at r1 (raw file):

Previously, orizi wrote…

this actually has possibly more clones than before still.

please answer at https://reviewable.io/reviews/starkware-libs/cairo/9692#-

it did not compile because you didn't change the callsite.

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