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

Deduplicate Clone by always using match and { } #31

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

ijackson
Copy link
Contributor

@ijackson ijackson commented Aug 1, 2024

This is the RFC MR I proposed in #16.

The overall effect is that we change to always use match and always use { field: ... } syntax, so for example tests::clone_struct::basic's expansion now contains this:

    struct Tuple(u8);
    impl ::core::clone::Clone for Tuple
...
        fn clone(&self) -> Self {
            match self {
                Self { 0: _s_0 } => {
                    Self {
                        0: ::core::clone::Clone::clone(_s_0),
                    }
                }
            }
        }

As I expected, using this approach allows the removal of a lot of code - and so far I've only treated Clone.

If you like this, I'll follow up with similar deduplication for the other traits.

@ijackson
Copy link
Contributor Author

ijackson commented Aug 1, 2024

FTAOD this implements, but only for Clone, these proposals from #16

  • "Always using match, unifying enum and struct code"
  • "Always using braced syntax"

I haven't tackled "Similarity between Ord, PartialOrd and PartialEq code". I think we should do the non-trait-specific deduplication first.

@ijackson
Copy link
Contributor Author

ijackson commented Aug 1, 2024

Oh, rustfmt. I will rewrite the branch.

No functional change, but this prepares the code for unification of
handling of named and unnamed fields.
Demonstrate the named-vs-unnamed-agnostic approach.

Unit, Named and Unnamed are all now handled by the named fields code,
using the `{ }` syntax which Rust allows for every `Data`.

The `match` statement is redundant, but removing it will involve a
deindent so is very textually invasive.  We'll do that in the next
commit.
Best reviewed with `git show -b`.
The clone_enum code is now good for cloning structs.  Use it.

The redundant block will be removed in a moment.
Best reviewed with `git show -b`
Apply deferred rewrapping.
@ijackson
Copy link
Contributor Author

ijackson commented Aug 1, 2024

"CI / Test 1.60 on ubuntu-latest () (pull_request) Failing after 20s" CI failure seems to be unrelated to MR contents.

IMO the root cause is not committing a suitable MSRV-compatible lockfile.

@ijackson
Copy link
Contributor Author

ijackson commented Aug 5, 2024

@magiclen do you like the rough shape of this? If so I can proceed with making similar changes to the other impls.

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.

1 participant