Skip to content

Conversation

@yanns
Copy link
Collaborator

@yanns yanns commented Oct 9, 2024

The first commit creates test coverage on state clone in fallback.
The second commit removes one clone.

This is an extraction of a part of #2865

@yanns yanns force-pushed the avoid_clone_in_fallback branch from f6733f6 to e2bbd5a Compare October 9, 2024 21:34
Comment on lines 6 to 8
pub(crate) struct CountingCloneableState {
state: Arc<Mutex<InnerState>>,
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍🏼

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually, we don't need a Mutex as we're using atomic values: d9ef8e0

@yanns
Copy link
Collaborator Author

yanns commented Oct 10, 2024

@jplatte do we prefer "squash and merge"?
Or I could make a better git log with only 2 commits, and use "rebase and merge"?

@jplatte
Copy link
Member

jplatte commented Oct 10, 2024

I'd say it doesn't matter much here. I implied in the other PR that squash doesn't preserve authorship, but actually I think GitHub will add Co-authored-by lines for separate authors in squash-merge commits.

Generally I default to squash-merge in axum bc. that's what David pretty much always used, making some exceptions for larger PRs with well-separated commits.

@yanns yanns force-pushed the avoid_clone_in_fallback branch from ce4c108 to 674f84d Compare October 10, 2024 07:55
@yanns
Copy link
Collaborator Author

yanns commented Oct 10, 2024

OK thanks. I'll keep the history here to show that the 2nd commit decreases the number of clones.

@yanns yanns enabled auto-merge (rebase) October 10, 2024 07:56
@yanns yanns merged commit 9feb526 into main Oct 10, 2024
18 checks passed
@yanns yanns deleted the avoid_clone_in_fallback branch October 10, 2024 08:01
@jplatte
Copy link
Member

jplatte commented Oct 10, 2024

One nitpick about the commit messages, please capitalize them in the future (unless there's some prefix like macros: , though I haven't seen that used much in this repo anyways).

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