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

Made all basic derives be based on members. #7321

Merged
merged 1 commit into from
Feb 25, 2025
Merged

Made all basic derives be based on members. #7321

merged 1 commit into from
Feb 25, 2025

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Feb 20, 2025

@orizi orizi mentioned this pull request Feb 20, 2025
@reviewable-StarkWare
Copy link

This change is Reviewable

@orizi orizi force-pushed the spr/main/602fc47a branch from 95c2be4 to 4af16ed Compare February 20, 2025 09:05
@orizi orizi changed the base branch from spr/main/f7201175 to main February 20, 2025 12:02
@orizi orizi force-pushed the spr/main/602fc47a branch from 4af16ed to d69f361 Compare February 20, 2025 12:02
@orizi orizi requested a review from mkaput as a code owner February 20, 2025 12:02
@mkaput mkaput removed their request for review February 20, 2025 12:07
@orizi orizi force-pushed the spr/main/602fc47a branch from d69f361 to f379979 Compare February 20, 2025 12:38
Copy link
Contributor

@gilbens-starkware gilbens-starkware 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 43 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-plugins/src/plugins/derive/mod.rs line 203 at r2 (raw file):

        let derived_trait_name = derived_trait.split("::").last().unwrap_or(derived_trait);
        format!(
            "impl {name}{derived_trait_name}<{generics}> of {derived_trait}::<{full_typename}>",

generics can be empty.

Code quote:

<{generics}>

Copy link
Collaborator Author

@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.

Reviewable status: 0 of 43 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @ilyalesokhin-starkware)


crates/cairo-lang-plugins/src/plugins/derive/mod.rs line 203 at r2 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

generics can be empty.

it can be - but doesn't really matter.

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware 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 43 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware)


corelib/src/internal.cairo line 21 at r2 (raw file):

}

/// Helper to have the same outside signature as `DropWith` and `DestructWith`.

what is an outside signature?

Code quote:

outside signature 

@orizi orizi force-pushed the spr/main/602fc47a branch from f379979 to 44a14d8 Compare February 23, 2025 08:33
Copy link
Collaborator Author

@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.

Reviewable status: 0 of 43 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @ilyalesokhin-starkware)


corelib/src/internal.cairo line 21 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

what is an outside signature?

Done
I mostly mean that it would look the same as the other 2 for users.
the word "outside" is useless here.
renamel to "PanicDestructInferred"

@orizi orizi force-pushed the spr/main/602fc47a branch 2 times, most recently from 0ca0006 to a94d43a Compare February 23, 2025 11:21
Copy link
Contributor

@gilbens-starkware gilbens-starkware 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 43 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-plugins/src/test_data/derive line 51 at r4 (raw file):

    a: A,
    b: B,
}

Suggestion:

#[derive(Clone, Debug, Default, Destruct, Hash, PanicDestruct, PartialEq, Serde)]
struct TwoMemberStruct {
    a: A,
    b: B,
}

#[derive(Clone, Debug, Default, Destruct, Hash, PanicDestruct, PartialEq, Serde)]
struct TwoSameTypeMemberStruct {
    a1: A,
    a2: A,
}

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware 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 43 files at r1.
Reviewable status: 1 of 43 files reviewed, 1 unresolved discussion (waiting on @orizi)

Copy link
Collaborator Author

@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.

Reviewable status: 1 of 43 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @ilyalesokhin-starkware)


crates/cairo-lang-plugins/src/test_data/derive line 51 at r4 (raw file):

    a: A,
    b: B,
}

Done.

Copy link
Contributor

@gilbens-starkware gilbens-starkware 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 23 of 43 files at r1, 2 of 3 files at r3, 16 of 16 files at r4, 2 of 2 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware 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 3 of 43 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @orizi)

@orizi orizi enabled auto-merge February 25, 2025 11:45
@orizi orizi force-pushed the spr/main/602fc47a branch from 0b23449 to d22307f Compare February 25, 2025 12:49
@orizi orizi added this pull request to the merge queue Feb 25, 2025
Merged via the queue into main with commit 783a015 Feb 25, 2025
47 checks passed
@orizi orizi deleted the spr/main/602fc47a branch February 25, 2025 13:23
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.

4 participants