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

Adding 'clone to satisfy the borrow checker' anti-pattern #110

Merged
merged 15 commits into from
Mar 29, 2021

Conversation

simonsan
Copy link
Collaborator

@simonsan simonsan commented Jan 1, 2021

Due to inactivity of the author and maintainers not being able to edit the PR I'll move #23 to a new branch in this repository.

Future fixes to the PR will be made on this branch.

@simonsan simonsan added the C-addition Category: Adding new content, something that didn't exist in the repository before label Jan 1, 2021
@simonsan simonsan force-pushed the borrow_clone_anti_pattern branch from 64b6a2e to 68f1793 Compare January 1, 2021 12:50
@pickfire
Copy link
Contributor

pickfire commented Jan 1, 2021

That said, I have seen quite a few blog posts discussing "it is okay to write inefficient code". Those posts even mentioned that clone can be used this way. I wonder if it should be put into the anti-pattern section, we could tell that we should reduce the usage of clone, but I think putting it in anti-pattern is going to make things hard (like what it did to way-cooler).

@simonsan
Copy link
Collaborator Author

simonsan commented Jan 1, 2021

That said, I have seen quite a few blog posts discussing "it is okay to write inefficient code". Those posts even mentioned that clone can be used this way. I wonder if it should be put into the anti-pattern section, we could tell that we should reduce the usage of clone, but I think putting it in anti-pattern is going to make things hard (like what it did to way-cooler).

I think it's fine to consider it an anti-pattern, as it's not trying to state that clone is inherently bad, but it is an anti-pattern in regards to satisfy the borrow checker. As long as it is explicitly stated and clear to the reader that this is the topic, it should be ok, I think.

@simonsan
Copy link
Collaborator Author

simonsan commented Jan 1, 2021

What I personally still miss here is the influence on performance of clone when used deliberately with bigger Vecs for example. I think that could be stated as a Disadvantage or outcome when using this anti-pattern often. While it should also explicitly state that clone is fine and what I mentioned before, the scope of this anti-pattern should be made crystal clear.

@LukeMathWalker
Copy link

My two cents - along the lines of what @pickfire mentioned: we should be very wary of demonizing .clone usage, particularly when we are talking about beginners.
At the same time, it makes sense to call out that "cloning your way out" might work as a short-term solution but might hide bigger issues with the whole architecture of your code that you might have to confront down the line anyway. Maybe linking a balanced article on the topic?

@simonsan
Copy link
Collaborator Author

simonsan commented Jan 2, 2021

Maybe linking a balanced article on the topic?

Do you have any recommendations?

@LukeMathWalker
Copy link

@simonsan simonsan force-pushed the borrow_clone_anti_pattern branch from c32561b to eaa9e0a Compare January 6, 2021 09:59
@simonsan simonsan added the A-anti_pattern Area: Content about Anti-Patterns label Jan 23, 2021
@Djazouli
Copy link

Hello !
Just to let you know, there is a note at the bottom of the file idioms/mem-replace.md, that referenced to PR #23 , and I believe the changes related to this note should then be included in this PR (I believe the only thing needed is to add a link to the newly created file)

@simonsan simonsan marked this pull request as ready for review March 29, 2021 18:55
@simonsan
Copy link
Collaborator Author

@pickfire @marcoieni Worked in your proposals, want to read over it again? ;)

@marcoieni
Copy link
Collaborator

That's great, thanks!

@simonsan simonsan merged commit c9c5f70 into master Mar 29, 2021
@simonsan simonsan deleted the borrow_clone_anti_pattern branch March 29, 2021 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-anti_pattern Area: Content about Anti-Patterns C-addition Category: Adding new content, something that didn't exist in the repository before
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants