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

Update FreeSurround #13194

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

tygyh
Copy link
Contributor

@tygyh tygyh commented Nov 24, 2024

Since FreeSurround seems to have been abandoned by its original author, I've decided to start maintaining it on my own to use it with Dolphin.

Most of the changes are refactorings and uses of modern c++. A few out-of-bound readings have also been fixed.

@JosJuice
Copy link
Member

I think forking a dependency just for the sake of general cleanup is unnecessary. We also don't have the capacity to review 1800 lines of minor changes.

@tygyh
Copy link
Contributor Author

tygyh commented Nov 24, 2024

I think forking a dependency just for the sake of general cleanup is unnecessary.

This is a continuation of #13169. The issue was external, so I fixed it externally. I can change this PR to not include any commit after the point when the vulnerability was fixed if that would make things easier for you

@JosJuice
Copy link
Member

As we previously explained to you, there was no vulnerability in the code you changed in PR #13169. Because of that, PR #13169 is just a minor modernization change, which isn't worth forking a dependency for.

@tygyh
Copy link
Contributor Author

tygyh commented Nov 24, 2024

PR #13169 is just a minor modernization change, which isn't worth forking a dependency for.

I think it is worth maintaining all parts of the codebase, even if it is imported from somewhere else. I've began adding doc comments to a few methods and will continue optimizing slow algorithms in my fork since there is no other active development.

The reason I'm continuing this discussion is because it doesn't matter how much I polish the code if no one uses it and since Dolphin already uses the old code I think it would benefit from using the updated code even though it has mostly just been cleaned up and only have a few performance improvements.

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