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

Utils/StringUtil: Fixes ltrim and adds a unittest #4171

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

Sonicadvance1
Copy link
Member

ltrim had the issue that it would always consume the left-most character even if it wasn't whitespace. So FEXLoader would turn in to EXLoader, even without any whitespace in the string.

Adds a test to ensure this doesn't occur again.

@pmatos
Copy link
Collaborator

pmatos commented Nov 26, 2024

Nice - lgtm. :)
We don't likely use this in any critical place; otherwise, I would wonder how anything works.

@pmatos
Copy link
Collaborator

pmatos commented Nov 26, 2024

Is there a reason we are not using std::erase_if from c++20, instead of erase+find_if?
https://en.cppreference.com/w/cpp/string/basic_string/erase2

Or the equivalent using std::remove_if, if we need to use iterators.

@Sonicadvance1
Copy link
Member Author

Is there a reason we are not using std::erase_if from c++20, instead of erase+find_if? https://en.cppreference.com/w/cpp/string/basic_string/erase2

Or the equivalent using std::remove_if, if we need to use iterators.

Haven't really looked at it, but I assume the predication would have different behaviour. I'm obviously not a very big C++ buff, using things for convenience rather than knowing every detail of it.

@pmatos
Copy link
Collaborator

pmatos commented Nov 26, 2024

Is there a reason we are not using std::erase_if from c++20, instead of erase+find_if? https://en.cppreference.com/w/cpp/string/basic_string/erase2
Or the equivalent using std::remove_if, if we need to use iterators.

Haven't really looked at it, but I assume the predication would have different behaviour. I'm obviously not a very big C++ buff, using things for convenience rather than knowing every detail of it.

Ah, actually I have tried it and it wouldn't work because erase_if would remove all white space instead of just trimming it, so it seems like your approach is the best unless you want to get into views and ranges etc. But it's simpler as you have it. Sorry for the noise.

@@ -0,0 +1,96 @@
#include <catch2/catch_test_macros.hpp>
#include <catch2/generators/catch_generators.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

this include is probably not needed

ltrim had the issue that it would always consume the left-most character
even if it wasn't whitespace. So `FEXLoader` would turn in to
`EXLoader`, even without any whitespace in the string.

Adds a test to ensure this doesn't occur again.
@Sonicadvance1 Sonicadvance1 merged commit 0463512 into FEX-Emu:main Nov 26, 2024
12 checks passed
@Sonicadvance1 Sonicadvance1 deleted the fix_ltrim branch November 26, 2024 21:09
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.

3 participants