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

@simonsan simonsan added this to In progress in Content Jan 2, 2021
@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 and others added 2 commits January 21, 2021 00:39
Co-authored-by: Marco Ieni <11428655+MarcoIeni@users.noreply.github.com>
Co-authored-by: Marco Ieni <11428655+MarcoIeni@users.noreply.github.com>
@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? ;)

Content automation moved this from In progress to Reviewer approved Mar 29, 2021
@MarcoIeni
Copy link
Collaborator

That's great, thanks!

@simonsan simonsan merged commit c9c5f70 into master Mar 29, 2021
Content automation moved this from Reviewer approved to Done 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
Development

Successfully merging this pull request may close these issues.

None yet

7 participants