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

rename to_arc to clone_arc #363

Merged
merged 1 commit into from
Nov 16, 2023
Merged

rename to_arc to clone_arc #363

merged 1 commit into from
Nov 16, 2023

Conversation

jsha
Copy link
Collaborator

@jsha jsha commented Nov 16, 2023

to_arc converts a *const C into an Option<Arc<T>>. It had a lot of verbiage to explain what it was doing with reference counts. I think the conceptual model becomes a lot simpler if we rename the function to reflect what it is really doing: cloning the arc.

@jsha jsha requested a review from cpu November 16, 2023 19:03
@jsha
Copy link
Collaborator Author

jsha commented Nov 16, 2023

Looks like tests are failing with possibly a crates.io outage:

error: failed to download from `[https://crates.io/api/v1/crates/spin/0.9.8/download`](https://crates.io/api/v1/crates/spin/0.9.8/download%60)

Status is 🍏 though. Will try again shortly.

src/acceptor.rs Outdated Show resolved Hide resolved
to_arc converts a `*const C` into an `Option<Arc<T>>`. It had a lot of
verbiage to explain what it was doing with reference counts. I think the
conceptual model becomes a lot simpler if we rename the function to
reflect what it is really doing: cloning the arc.
Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

LGTM! I like the new name 👍

Hopefully CI recovers soon 🤞 In the meantime I built + tested locally w/o any issues.

@jsha jsha merged commit e5e09a0 into main Nov 16, 2023
42 checks passed
@jsha jsha deleted the try-clone-arc branch November 16, 2023 21:54
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.

2 participants