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

Add String::remove_char(s) methods for performance and convenience #92476

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented May 28, 2024

Companion to:

With essentially the same logic and considerations, see there for more reasoning. Will also perform benchmarks on this PR and see about exposing these if desired.

Named it remove as the existing erase method is very different so wanting to avoid confusion between the two.

@AThousandShips AThousandShips added this to the 4.x milestone May 28, 2024
@AThousandShips AThousandShips changed the title String remove char Add String::remove_char(s) methods for performance and convenience May 28, 2024
@@ -1849,7 +1849,7 @@ void EditorHelp::_update_doc() {
_push_code_font();

if (constant.value.begins_with("Color(") && constant.value.ends_with(")")) {
String stripped = constant.value.replace(" ", "").replace("Color(", "").replace(")", "");
String stripped = constant.value.remove_char(' ').replace("Color(", "").remove_char(')');
Copy link
Member Author

Choose a reason for hiding this comment

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

Could change this to:

Suggested change
String stripped = constant.value.remove_char(' ').replace("Color(", "").remove_char(')');
String stripped = constant.value.remove_chars({ ' ', ')' }).replace("Color(", "");

But since the original code keeps the two removes separate I'm unsure if it might cause some issues so left it for now, but if it's considered safe I'll change this

@AThousandShips
Copy link
Member Author

Added an alternative approach based on the changes in:

@AThousandShips AThousandShips force-pushed the string_remove_char branch 3 times, most recently from 43828aa to d1e2a8e Compare December 19, 2024 17:44
@AThousandShips AThousandShips requested a review from a team as a code owner December 22, 2024 22:09
@AThousandShips
Copy link
Member Author

Reworked with the same logic as the replace_char(s) code, and exposed, see there for more details (will also port the _contains_char helper method to that one too so it can be a shared helper method)

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

Successfully merging this pull request may close these issues.

2 participants