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.

Sign up for GitHub

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

Jump to bottom

Replace String.substr() with simpler functions where possible #82412

Closed
wants to merge 1 commit into from
Closed

Replace String.substr() with simpler functions where possible #82412

wants to merge 1 commit into from

Conversation

MewPurPur
Copy link
Contributor

substr(0, len) can be simplified to left(len) if len >= 0. In fact, this alternative is 10% faster.

While this is mostly about code style, I expect the performance improvement to be subtle, but non-negligible. Since String also used substr() in a few places when it could use left(), this should optimize other String functions too, and those are used all over the codebase, too.

And a couple of other code simplifications.

@MewPurPur MewPurPur requested review from a team as code owners September 26, 2023 22:28
@MewPurPur MewPurPur force-pushed the STOP-POSTING-ABOUT-substr-IM-TIRED-OF-SEEING-IT-I-WAS-IN-A-SOURCE-FILE-RIGHT-AND-AAALLLL-THE-MANIPULATIONS-ARE-JUST-substr-STUFF branch 2 times, most recently from 914e412 to 02fbf8f Compare September 26, 2023 22:31
@KoBeWi KoBeWi added this to the 4.x milestone Sep 26, 2023
@MewPurPur MewPurPur force-pushed the STOP-POSTING-ABOUT-substr-IM-TIRED-OF-SEEING-IT-I-WAS-IN-A-SOURCE-FILE-RIGHT-AND-AAALLLL-THE-MANIPULATIONS-ARE-JUST-substr-STUFF branch 3 times, most recently from 2545b46 to 92da4d3 Compare September 27, 2023 00:03
@MewPurPur
Copy link
Contributor Author

MewPurPur commented Sep 27, 2023 •
edited
Loading

I'll proof-read it now and leave comments to make reviewing easier.

@MewPurPur MewPurPur force-pushed the STOP-POSTING-ABOUT-substr-IM-TIRED-OF-SEEING-IT-I-WAS-IN-A-SOURCE-FILE-RIGHT-AND-AAALLLL-THE-MANIPULATIONS-ARE-JUST-substr-STUFF branch from 92da4d3 to 9330273 Compare September 27, 2023 09:31
MewPurPur
MewPurPur commented Sep 27, 2023
View reviewed changes
scene/main/node.cpp
@@ -1330,7 +1330,7 @@ void Node::_generate_serial_child_name(const Node *p_child, StringName &name) co

// Assign the base name + separator to name if we have numbers preceded by a separator
if (nums.length() > 0 && name_string.substr(name_last_index, nnsep.length()) == nnsep) {
name_string = name_string.substr(0, name_last_index + nnsep.length());
name_string = name_string.left(name_last_index + nnsep.length());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is complex, but in a nutshell, I haven't determined if my change affects any behaviors. But if it does, then I'm 99% sure the old behavior would've also been problematic.

@MewPurPur MewPurPur force-pushed the STOP-POSTING-ABOUT-substr-IM-TIRED-OF-SEEING-IT-I-WAS-IN-A-SOURCE-FILE-RIGHT-AND-AAALLLL-THE-MANIPULATIONS-ARE-JUST-substr-STUFF branch from 295b4bf to f3b5206 Compare September 27, 2023 22:05
@MewPurPur MewPurPur force-pushed the STOP-POSTING-ABOUT-substr-IM-TIRED-OF-SEEING-IT-I-WAS-IN-A-SOURCE-FILE-RIGHT-AND-AAALLLL-THE-MANIPULATIONS-ARE-JUST-substr-STUFF branch from f3b5206 to 64b4d71 Compare September 27, 2023 22:44
@MewPurPur MewPurPur force-pushed the STOP-POSTING-ABOUT-substr-IM-TIRED-OF-SEEING-IT-I-WAS-IN-A-SOURCE-FILE-RIGHT-AND-AAALLLL-THE-MANIPULATIONS-ARE-JUST-substr-STUFF branch 2 times, most recently from 32878e4 to 8f37d8d Compare September 28, 2023 09:57
@MewPurPur MewPurPur force-pushed the STOP-POSTING-ABOUT-substr-IM-TIRED-OF-SEEING-IT-I-WAS-IN-A-SOURCE-FILE-RIGHT-AND-AAALLLL-THE-MANIPULATIONS-ARE-JUST-substr-STUFF branch from 8f37d8d to ea873f7 Compare September 28, 2023 10:30
@MewPurPur MewPurPur force-pushed the STOP-POSTING-ABOUT-substr-IM-TIRED-OF-SEEING-IT-I-WAS-IN-A-SOURCE-FILE-RIGHT-AND-AAALLLL-THE-MANIPULATIONS-ARE-JUST-substr-STUFF branch from ea873f7 to bcf568a Compare September 28, 2023 11:04
@MewPurPur
Copy link
Contributor Author

I've self-reviewed this so extensively I don't think a second major review would be needed P: I'll finish this soon and comment on which changes I'm still skeptical about.

@MewPurPur
Copy link
Contributor Author

MewPurPur commented Dec 11, 2023 •
edited
Loading

I want to go about this differently.

@MewPurPur MewPurPur closed this Dec 11, 2023
@YeldhamDev YeldhamDev removed this from the 4.x milestone Dec 11, 2023
@MewPurPur MewPurPur mentioned this pull request Dec 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YuriSizov YuriSizov YuriSizov left review comments

Assignees
No one assigned
Labels
archived enhancement performance topic:codestyle
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MewPurPur @YuriSizov @KoBeWi @Chaosus @YeldhamDev