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

refactor: remove unnecessary to_string() #99

Merged
merged 3 commits into from
Mar 23, 2024

Conversation

murlakatamenka
Copy link

aint-much

@Shinyzenith
Copy link
Member

Respect the honest work. All new patches need to be rebased onto freeze-feat-andreas as that's the current dev branch with breaking changes. It will soon be merged into main.

@murlakatamenka
Copy link
Author

@Shinyzenith yeah, no problems here.

While at it, I'll push more commits to fix current clippy warnings, CONTRIBUTING.md and make check expect clean output.

@murlakatamenka
Copy link
Author

Okay, it's a beefier PR now and ready to be reviewed.

@Shinyzenith
Copy link
Member

Hi the pr target needs to be changed to the above mentioned branch.

@Shinyzenith
Copy link
Member

Also I see you changed the previous commits of the branch which isn't ideal as their signature in the git object store will change.

Typically `&Vec<T>` isn't something you want when you need a read-only view
over a Vec, because it adds another level of indirection: Vec already stores
a pointer to heap-allocated buffer internally.

Using slices `&[T]` removes such unnecessary level of indirection and
is considered a cleaner design. It is cache friendlier and can be better
optimized by the compiler (not that it should matter in this case).
@murlakatamenka murlakatamenka changed the base branch from main to freeze-feat-andreas March 14, 2024 17:09
@murlakatamenka
Copy link
Author

Yeah, sorry for the fuss.

The order is restored now, the target branch is that feature freeze one.

@Shinyzenith Shinyzenith merged commit f53e650 into waycrate:freeze-feat-andreas Mar 23, 2024
3 checks passed
@Shinyzenith
Copy link
Member

Thank you for the patch! It's definitely honest work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants