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::replace_char(s) methods for performance and convenience #92475

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions core/config/project_settings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ String ProjectSettings::localize_path(const String &p_path) const {

if (dir->change_dir(path) == OK) {
String cwd = dir->get_current_dir();
cwd = cwd.replace("\\", "/");
cwd = cwd.replace_char('\\', '/');

// Ensure that we end with a '/'.
// This is important to ensure that we do not wrongly localize the resource path
Expand Down Expand Up @@ -563,7 +563,7 @@ Error ProjectSettings::_setup(const String &p_path, const String &p_main_pack, b
if (!OS::get_singleton()->get_resource_dir().is_empty()) {
// OS will call ProjectSettings->get_resource_path which will be empty if not overridden!
// If the OS would rather use a specific location, then it will not be empty.
resource_path = OS::get_singleton()->get_resource_dir().replace("\\", "/");
resource_path = OS::get_singleton()->get_resource_dir().replace_char('\\', '/');
if (!resource_path.is_empty() && resource_path[resource_path.length() - 1] == '/') {
resource_path = resource_path.substr(0, resource_path.length() - 1); // Chop end.
}
Expand Down Expand Up @@ -662,7 +662,7 @@ Error ProjectSettings::_setup(const String &p_path, const String &p_main_pack, b
while (true) {
// Set the resource path early so things can be resolved when loading.
resource_path = current_dir;
resource_path = resource_path.replace("\\", "/"); // Windows path to Unix path just in case.
resource_path = resource_path.replace_char('\\', '/'); // Windows path to Unix path just in case.
err = _load_settings_text_or_binary(current_dir.path_join("project.godot"), current_dir.path_join("project.binary"));
if (err == OK && !p_ignore_override) {
// Optional, we don't mind if it fails.
Expand Down
6 changes: 3 additions & 3 deletions core/doc_data.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@

String DocData::get_default_value_string(const Variant &p_value) {
if (p_value.get_type() == Variant::ARRAY) {
return Variant(Array(p_value, 0, StringName(), Variant())).get_construct_string().replace("\n", " ");
return Variant(Array(p_value, 0, StringName(), Variant())).get_construct_string().replace_char('\n', ' ');
} else if (p_value.get_type() == Variant::DICTIONARY) {
return Variant(Dictionary(p_value, 0, StringName(), Variant(), 0, StringName(), Variant())).get_construct_string().replace("\n", " ");
return Variant(Dictionary(p_value, 0, StringName(), Variant(), 0, StringName(), Variant())).get_construct_string().replace_char('\n', ' ');
} else {
return p_value.get_construct_string().replace("\n", " ");
return p_value.get_construct_string().replace_char('\n', ' ');
}
}

Expand Down
2 changes: 1 addition & 1 deletion core/io/dir_access.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ Error DirAccess::make_dir_recursive(const String &p_dir) {
full_dir = p_dir;
}

full_dir = full_dir.replace("\\", "/");
full_dir = full_dir.replace_char('\\', '/');

String base;

Expand Down
2 changes: 1 addition & 1 deletion core/io/file_access.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ FileAccess::AccessType FileAccess::get_access_type() const {
String FileAccess::fix_path(const String &p_path) const {
// Helper used by file accesses that use a single filesystem.

String r_path = p_path.replace("\\", "/");
String r_path = p_path.replace_char('\\', '/');

switch (_access_type) {
case ACCESS_RESOURCES: {
Expand Down
2 changes: 1 addition & 1 deletion core/io/file_access_pack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ String DirAccessPack::get_drive(int p_drive) {
}

PackedData::PackedDir *DirAccessPack::_find_dir(const String &p_dir) {
String nd = p_dir.replace("\\", "/");
String nd = p_dir.replace_char('\\', '/');

// Special handling since simplify_path() will forbid it
if (p_dir == "..") {
Expand Down
2 changes: 1 addition & 1 deletion core/io/image.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2297,7 +2297,7 @@ void Image::initialize_data(const char **p_xpm) {
switch (status) {
case READING_HEADER: {
String line_str = line_ptr;
line_str.replace("\t", " ");
line_str.replace_char('\t', ' ');

size_width = line_str.get_slicec(' ', 0).to_int();
size_height = line_str.get_slicec(' ', 1).to_int();
Expand Down
2 changes: 1 addition & 1 deletion core/io/logger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ void RotatedFileLogger::rotate_file() {

if (FileAccess::exists(base_path)) {
if (max_files > 1) {
String timestamp = Time::get_singleton()->get_datetime_string_from_system().replace(":", ".");
String timestamp = Time::get_singleton()->get_datetime_string_from_system().replace_char(':', '.');
String backup_name = base_path.get_basename() + timestamp;
if (!base_path.get_extension().is_empty()) {
backup_name += "." + base_path.get_extension();
Expand Down
2 changes: 1 addition & 1 deletion core/os/os.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ String OS::get_safe_dir_name(const String &p_dir_name, bool p_allow_paths) const
if (p_allow_paths) {
// Dir separators are allowed, but disallow ".." to avoid going up the filesystem
invalid_chars.push_back("..");
safe_dir_name = safe_dir_name.replace("\\", "/").strip_edges();
safe_dir_name = safe_dir_name.replace_char('\\', '/').strip_edges();
} else {
invalid_chars.push_back("/");
invalid_chars.push_back("\\");
Expand Down
2 changes: 1 addition & 1 deletion core/string/translation_server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ TranslationServer::Locale::operator String() const {

TranslationServer::Locale::Locale(const TranslationServer &p_server, const String &p_locale, bool p_add_defaults) {
// Replaces '-' with '_' for macOS style locales.
String univ_locale = p_locale.replace("-", "_");
String univ_locale = p_locale.replace_char('-', '_');

// Extract locale elements.
Vector<String> locale_elements = univ_locale.get_slice("@", 0).split("_");
Expand Down
121 changes: 114 additions & 7 deletions core/string/ustring.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1015,7 +1015,7 @@ String String::_camelcase_to_underscore() const {
}

String String::capitalize() const {
String aux = _camelcase_to_underscore().replace("_", " ").strip_edges();
String aux = _camelcase_to_underscore().replace_char('_', ' ').strip_edges();
String cap;
for (int i = 0; i < aux.get_slice_count(" "); i++) {
String slice = aux.get_slicec(' ', i);
Expand Down Expand Up @@ -1044,7 +1044,7 @@ String String::to_pascal_case() const {
}

String String::to_snake_case() const {
return _camelcase_to_underscore().replace(" ", "_").strip_edges();
return _camelcase_to_underscore().replace_char(' ', '_').strip_edges();
}

String String::get_with_code_lines() const {
Expand Down Expand Up @@ -3107,6 +3107,17 @@ String String::erase(int p_pos, int p_chars) const {
return left(p_pos) + substr(p_pos + p_chars);
}

template <class T>
static bool _contains_char(char32_t p_c, const T *p_chars, int p_chars_len) {
for (int i = 0; i < p_chars_len; ++i) {
if (p_c == (char32_t)p_chars[i]) {
return true;
}
}

return false;
}

String String::substr(int p_from, int p_chars) const {
if (p_chars == -1) {
p_chars = length() - p_from;
Expand Down Expand Up @@ -4143,6 +4154,102 @@ String String::replace_first(const char *p_key, const char *p_with) const {
return *this;
}

String String::replace_char(char32_t p_key, char32_t p_with) const {
ERR_FAIL_COND_V_MSG(p_with == 0, *this, "`with` must not be the NUL character.");

int len = length();
if (p_key == 0 || len == 0) {
AThousandShips marked this conversation as resolved.
Show resolved Hide resolved
return *this;
}

int index = find_char(p_key);

// If no occurrence of `key` was found, return this.
if (index == -1) {
return *this;
}

const char32_t *old_ptr = ptr();

// If we found at least one occurrence of `key`, create new string.
String new_string;
new_string.resize(len + 1);
char32_t *new_ptr = new_string.ptrw();

// Copy part of input before `key`.
memcpy(new_ptr, old_ptr, index * sizeof(char32_t));

new_ptr[index] = p_with;

// Copy or replace rest of input.
for (++index; index < len; ++index) {
if (old_ptr[index] == p_key) {
new_ptr[index] = p_with;
} else {
new_ptr[index] = old_ptr[index];
Comment on lines +4186 to +4189
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
if (old_ptr[index] == p_key) {
new_ptr[index] = p_with;
} else {
new_ptr[index] = old_ptr[index];
const char32_t old_char = old_ptr[index];
if (old_char == p_key) {
new_ptr[index] = p_with;
} else {
new_ptr[index] = old_char;

Possible improvement to avoid accessing twice, but should be optimized out likely

}
}

new_ptr[index] = _null;

return new_string;
}

template <class T>
static String _replace_chars_common(const String &p_this, const T *p_keys, int p_keys_len, char32_t p_with) {
ERR_FAIL_COND_V_MSG(p_with == 0, p_this, "`with` must not be the NUL character.");
Copy link
Member Author

Choose a reason for hiding this comment

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

Will move this to the individual functions for easier use, but away from the computer right now


int len = p_this.length();
if (p_keys_len == 0 || len == 0) {
return p_this;
}

Copy link
Contributor

@Ivorforce Ivorforce Dec 21, 2024

Choose a reason for hiding this comment

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

What about my earlier suggestion to add this here?

if (p_keys.size() == 1) {
    return replace_char(p_keys[0], p_with;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point can test

int index = 0;
const char32_t *old_ptr = p_this.ptr();
for (; index < len; ++index) {
if (_contains_char(old_ptr[index], p_keys, p_keys_len)) {
break;
}
}

// If no occurrence of `keys` was found, return this.
if (index == len) {
return p_this;
}

// If we found at least one occurrence of `keys`, create new string.
String new_string;
new_string.resize(len + 1);
char32_t *new_ptr = new_string.ptrw();

// Copy part of input before `key`.
memcpy(new_ptr, old_ptr, index * sizeof(char32_t));

new_ptr[index] = p_with;

// Copy or replace rest of input.
for (++index; index < len; ++index) {
const char32_t old_char = old_ptr[index];
if (_contains_char(old_char, p_keys, p_keys_len)) {
new_ptr[index] = p_with;
} else {
new_ptr[index] = old_char;
}
}

new_ptr[index] = 0;

return new_string;
}

String String::replace_chars(const String &p_keys, char32_t p_with) const {
return _replace_chars_common(*this, p_keys.ptr(), p_keys.length(), p_with);
}

String String::replace_chars(const char *p_keys, char32_t p_with) const {
return _replace_chars_common(*this, p_keys, strlen(p_keys), p_with);
}

String String::replacen(const String &p_key, const String &p_with) const {
return _replace_common(*this, p_key, p_with, true);
}
Expand Down Expand Up @@ -4425,7 +4532,7 @@ String String::simplify_path() const {
}
}

s = s.replace("\\", "/");
s = s.replace_char('\\', '/');
while (true) { // in case of using 2 or more slash
String compare = s.replace("//", "/");
if (s == compare) {
Expand Down Expand Up @@ -5041,8 +5148,8 @@ bool String::is_valid_float() const {

String String::path_to_file(const String &p_path) const {
// Don't get base dir for src, this is expected to be a dir already.
String src = replace("\\", "/");
String dst = p_path.replace("\\", "/").get_base_dir();
String src = replace_char('\\', '/');
String dst = p_path.replace_char('\\', '/').get_base_dir();
String rel = src.path_to(dst);
if (rel == dst) { // failed
return p_path;
Expand All @@ -5052,8 +5159,8 @@ String String::path_to_file(const String &p_path) const {
}

String String::path_to(const String &p_path) const {
String src = replace("\\", "/");
String dst = p_path.replace("\\", "/");
String src = replace_char('\\', '/');
String dst = p_path.replace_char('\\', '/');
if (!src.ends_with("/")) {
src += "/";
}
Expand Down
3 changes: 3 additions & 0 deletions core/string/ustring.h
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,9 @@ class String {
String replace_first(const char *p_key, const char *p_with) const;
String replace(const String &p_key, const String &p_with) const;
String replace(const char *p_key, const char *p_with) const;
String replace_char(char32_t p_key, char32_t p_with) const;
String replace_chars(const String &p_keys, char32_t p_with) const;
String replace_chars(const char *p_keys, char32_t p_with) const;
Copy link
Member Author

Choose a reason for hiding this comment

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

Can easily make a replace_chars(const Vector<char32_t> &, char32_t) version as well, it can use the same method, but leaving with this for now

Copy link
Contributor

Choose a reason for hiding this comment

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

No need i think, there's little chance somebody even owns vector<char32_t> instead of String.

String replacen(const String &p_key, const String &p_with) const;
String replacen(const char *p_key, const char *p_with) const;
String repeat(int p_count) const;
Expand Down
2 changes: 2 additions & 0 deletions core/variant/variant_call.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1713,6 +1713,8 @@ static void _register_variant_builtin_methods_string() {
bind_string_method(format, sarray("values", "placeholder"), varray("{_}"));
bind_string_methodv(replace, static_cast<String (String::*)(const String &, const String &) const>(&String::replace), sarray("what", "forwhat"), varray());
bind_string_methodv(replacen, static_cast<String (String::*)(const String &, const String &) const>(&String::replacen), sarray("what", "forwhat"), varray());
bind_string_method(replace_char, sarray("key", "with"), varray());
bind_string_methodv(replace_chars, static_cast<String (String::*)(const String &, char32_t) const>(&String::replace_chars), sarray("keys", "with"), varray());
bind_string_method(repeat, sarray("count"), varray());
bind_string_method(reverse, sarray(), varray());
bind_string_method(insert, sarray("position", "what"), varray());
Expand Down
16 changes: 16 additions & 0 deletions doc/classes/String.xml
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,22 @@
Replaces all occurrences of [param what] inside the string with the given [param forwhat].
</description>
</method>
<method name="replace_char" qualifiers="const">
<return type="String" />
<param index="0" name="key" type="int" />
<param index="1" name="with" type="int" />
<description>
Replaces all occurrences of the Unicode character with code [param key] with the Unicode character with code [param with]. Faster version of [method replace] when the key is only one character long.
</description>
</method>
<method name="replace_chars" qualifiers="const">
<return type="String" />
<param index="0" name="keys" type="String" />
<param index="1" name="with" type="int" />
<description>
Replaces any occurrence of the characters in [param keys] with the Unicode character with code [param with]. See also [method replace_char].
</description>
</method>
<method name="replacen" qualifiers="const">
<return type="String" />
<param index="0" name="what" type="String" />
Expand Down
16 changes: 16 additions & 0 deletions doc/classes/StringName.xml
Original file line number Diff line number Diff line change
Expand Up @@ -686,6 +686,22 @@
Replaces all occurrences of [param what] inside the string with the given [param forwhat].
</description>
</method>
<method name="replace_char" qualifiers="const">
<return type="String" />
<param index="0" name="key" type="int" />
<param index="1" name="with" type="int" />
<description>
Replaces all occurrences of the Unicode character with code [param key] with the Unicode character with code [param with]. Faster version of [method replace] when the key is only one character long.
</description>
</method>
<method name="replace_chars" qualifiers="const">
<return type="String" />
<param index="0" name="keys" type="String" />
<param index="1" name="with" type="int" />
<description>
Replaces any occurrence of the characters in [param keys] with the Unicode character with code [param with]. See also [method replace_char].
</description>
</method>
<method name="replacen" qualifiers="const">
<return type="String" />
<param index="0" name="what" type="String" />
Expand Down
4 changes: 2 additions & 2 deletions drivers/egl/egl_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ int EGLManager::_get_gldisplay_id(void *p_display) {
String EGLManager::shader_cache_dir;

void EGLManager::_set_cache(const void *p_key, EGLsizeiANDROID p_key_size, const void *p_value, EGLsizeiANDROID p_value_size) {
String name = CryptoCore::b64_encode_str((const uint8_t *)p_key, p_key_size).replace("/", "_");
String name = CryptoCore::b64_encode_str((const uint8_t *)p_key, p_key_size).replace_char('/', '_');
String path = shader_cache_dir.path_join(name) + ".cache";

Error err = OK;
Expand All @@ -161,7 +161,7 @@ void EGLManager::_set_cache(const void *p_key, EGLsizeiANDROID p_key_size, const
}

EGLsizeiANDROID EGLManager::_get_cache(const void *p_key, EGLsizeiANDROID p_key_size, void *p_value, EGLsizeiANDROID p_value_size) {
String name = CryptoCore::b64_encode_str((const uint8_t *)p_key, p_key_size).replace("/", "_");
String name = CryptoCore::b64_encode_str((const uint8_t *)p_key, p_key_size).replace_char('/', '_');
String path = shader_cache_dir.path_join(name) + ".cache";

Error err = OK;
Expand Down
2 changes: 1 addition & 1 deletion drivers/metal/rendering_device_driver_metal.mm
Original file line number Diff line number Diff line change
Expand Up @@ -3030,7 +3030,7 @@ bool isArrayTexture(MTLTextureType p_type) {

String RenderingDeviceDriverMetal::_pipeline_get_cache_path() const {
String path = OS::get_singleton()->get_user_data_dir() + "/metal/pipelines";
path += "." + context_device.name.validate_filename().replace(" ", "_").to_lower();
path += "." + context_device.name.validate_filename().replace_char(' ', '_').to_lower();
if (Engine::get_singleton()->is_editor_hint()) {
path += ".editor";
}
Expand Down
2 changes: 1 addition & 1 deletion drivers/unix/file_access_unix_pipe.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ Error FileAccessUnixPipe::open_internal(const String &p_path, int p_mode_flags)
path_src = p_path;
ERR_FAIL_COND_V_MSG(fd[0] >= 0 || fd[1] >= 0, ERR_ALREADY_IN_USE, "Pipe is already in use.");

path = String("/tmp/") + p_path.replace("pipe://", "").replace("/", "_");
path = String("/tmp/") + p_path.replace("pipe://", "").replace_char('/', '_');
struct stat st = {};
int err = stat(path.utf8().get_data(), &st);
if (err) {
Expand Down
Loading