godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.1k stars 21.18k forks source link

Use String::plus_file() instead of `[+=] "/" + `? #29602

Closed NilsIrl closed 5 years ago

NilsIrl commented 5 years ago

Issue description:

On many instances, [+=] "/" + is used instead of plus_file(). I was told in https://github.com/godotengine/godot/pull/29198#discussion_r287728562 to use it over [+=] "/" + just for consistency. Should this be changed throughout the code base?

Please keep in mind that this grep output may have many false positives (The type isn't even a String or it doesn't semantically make sense to change anything).

$ git grep '[+=] "/" + '
SConstruct:                     env.doc_class_path[c] = "modules/" + x + "/" + doc_path
core/node_path.cpp:         initial_subname += "/" + data->path[i];
core/os/dir_access.cpp:             Error err = copy(get_current_dir() + "/" + n, p_to + rel_path, p_chmod_flags);
core/project_settings.cpp:                  set(section + "/" + assign, value);
core/project_settings.cpp:              key = E->key() + "/" + key;
core/project_settings.cpp:              key = E->key() + "/" + key;
core/script_debugger_local.cpp: print_line("FRAME: total: " + rtos(frame_time) + " script: " + rtos(script_time) + "/" + itos(script_time * 100 / total_time) + " %");
core/script_debugger_local.cpp:     print_line("\ttotal: " + rtos(tt) + "/" + itos(tt * 100 / total_time) + " % \tself: " + rtos(st) + "/" + itos(st * 100 / total_time) + " % tcalls: " + itos(pinfo[i].call_count));
core/ustring.cpp:   return *this + "/" + p_file;
drivers/unix/dir_access_unix.cpp:       String next_dir = current_dir + "/" + p_dir;
editor/collada/collada.cpp:         image.path = ProjectSettings::get_singleton()->localize_path(state.local_path.get_base_dir() + "/" + path.percent_decode());
editor/collada/collada.cpp:                     path = ProjectSettings::get_singleton()->localize_path(state.local_path.get_base_dir() + "/" + path);
editor/editor_file_system.cpp:      p = d->name + "/" + p;
editor/editor_file_system.cpp:      file = d->name + "/" + file;
editor/editor_help.cpp:         Ref<Texture> texture = ResourceLoader::load(base_path + "/" + image, "Texture");
editor/editor_help.cpp:         Ref<Font> font = ResourceLoader::load(base_path + "/" + fnt, "Font");
editor/editor_inspector.cpp:            basename = group + "/" + basename;
editor/editor_node.cpp: path = "res://addons/" + p_addon + "/" + path;
editor/editor_sectioned_inspector.cpp:          name = section + "/" + name;
editor/editor_sectioned_inspector.cpp:          name = section + "/" + name;
editor/editor_sectioned_inspector.cpp:      return edited->call("property_can_revert", section + "/" + p_name);
editor/editor_sectioned_inspector.cpp:      return edited->call("property_get_revert", section + "/" + p_name);
editor/editor_sectioned_inspector.cpp:      return base + "/" + p_item;
editor/editor_sectioned_inspector.cpp:              metasection += "/" + sectionarr[i];
editor/editor_settings.cpp:         list.write[i] = exe_path + "/" + list[i];
editor/export_template_manager.cpp:                 status += " " + String::humanize_size(download_templates->get_downloaded_bytes()) + "/" + String::humanize_size(download_templates->get_body_size());
editor/filesystem_dock.cpp:     new_path = base_dir.substr(0, base_dir.find_last("/")) + "/" + new_name;
editor/groups_editor.cpp:       item_name = String(p_current->get_parent()->get_name()) + "/" + String(item_name);
editor/import/resource_importer_scene.cpp:              progress2.step(TTR("Generating for Mesh: ") + name + " (" + itos(step) + "/" + itos(meshes.size()) + ")", step);
editor/plugins/animation_blend_tree_editor_plugin.cpp:          String base_path = AnimationTreeEditor::get_singleton()->get_base_path() + String(E->get()) + "/" + F->get().name;
editor/plugins/animation_state_machine_editor.cpp:                  next += "/" + current_node_playback->get_current_node();
editor/plugins/asset_library_editor_plugin.cpp: request->request(host + "/" + p_request + p_arguments);
editor/plugins/canvas_item_editor_plugin.cpp:                   String node_path = "/" + root_name + "/" + root_path.rel_path_to(item->get_path());
editor/plugins/canvas_item_editor_plugin.cpp:       editor_data->get_undo_redo().add_undo_method(sed, "live_debug_remove_node", NodePath(String(editor->get_edited_scene()->get_path_to(parent)) + "/" + new_name));
editor/plugins/canvas_item_editor_plugin.cpp:   editor_data->get_undo_redo().add_undo_method(sed, "live_debug_remove_node", NodePath(String(editor->get_edited_scene()->get_path_to(parent)) + "/" + new_name));
editor/plugins/script_text_editor.cpp:          String source_path = base == connection.source ? base_path : base_path + "/" + String(base->get_path_to(Object::cast_to<Node>(connection.source)));
editor/plugins/script_text_editor.cpp:          String target_path = base == connection.target ? base_path : base_path + "/" + String(base->get_path_to(Object::cast_to<Node>(connection.target)));
editor/plugins/spatial_editor_plugin.cpp:           String node_path = "/" + root_name + "/" + root_path.rel_path_to(spat->get_path());
editor/plugins/spatial_editor_plugin.cpp:   editor_data->get_undo_redo().add_undo_method(sed, "live_debug_remove_node", NodePath(String(editor->get_edited_scene()->get_path_to(parent)) + "/" + new_name));
editor/plugins/tile_set_editor_plugin.cpp:          tileset->set(String::num(tileset_editor->get_current_tile(), 0) + "/" + name2, p_value, &v);
editor/plugins/tile_set_editor_plugin.cpp:          r_ret = tileset->get(String::num(tileset_editor->get_current_tile(), 0) + "/" + name, &v);
editor/project_settings_editor.cpp: String name = catname + "/" + propname;
editor/script_editor_debugger.cpp:      path = "/" + lp + path;
editor/script_editor_debugger.cpp:                  text = ti->get_text(0) + "/" + text;
gles_builders.py:            included_file = os.path.relpath(os.path.dirname(filename) + "/" + includeline)
main/main.cpp:                          local_game_path = da->get_current_dir() + "/" + local_game_path;
main/main.cpp:                              local_game_path = da->get_current_dir() + "/" + local_game_path.substr(sep + 1, local_game_path.length());
main/tests/test_gdscript.cpp:                   txt = _parser_expr(c_node->arguments[0]) + "/" + _parser_expr(c_node->arguments[1]);
main/tests/test_math.cpp:                   print_line("success at " + itos(i) + "/" + itos(nearest_shift(hashes.size())) + " shift " + itos(s));
methods.py:        filetype = sorted(glob.glob(dir_path + "/" + filetype))
methods.py:            pngf = open(x + "/" + name + ".png", "rb")
methods.py:            wf = x + "/" + name + ".gen.h"
modules/assimp/editor_scene_importer_assimp.cpp:            String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:            String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:            String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:            String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:        String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:            String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:            String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:            String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:            String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:            String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:        String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:            String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:            String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:            String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:            String path = state.path.get_base_dir() + "/" + filename.replace("\\", "/");
modules/assimp/editor_scene_importer_assimp.cpp:    String name_ignore_sub_directory = p_path.get_base_dir() + "/" + path.get_file().get_basename() + extension;
modules/gdscript/gdscript_parser.cpp:               path = base_path + "/" + path;
modules/gdscript/gdscript_parser.cpp:                           hint_prefix += "/" + itos(current_export.hint);
modules/mono/csharp_script.cpp:     r_hint_string = itos(elem_variant_type) + "/" + itos(elem_hint) + ":" + elem_hint_string;
modules/mono/glue/Managed/Files/StringExtensions.cs:            return instance + "/" + file;
modules/opus/SCsub:    env_opus.Prepend(CPPPATH=[thirdparty_dir + "/" + dir for dir in thirdparty_include_paths])
modules/visual_script/visual_script_editor.cpp: String node_name = "custom/" + p_category + "/" + p_name;
modules/visual_script/visual_script_editor.cpp: String node_name = "custom/" + p_category + "/" + p_name;
modules/visual_script/visual_script_func_nodes.cpp:         VisualScriptLanguage::singleton->add_register_func("functions/by_type/" + type_name + "/" + E->get().name, create_basic_type_call_node);
platform/android/detect.py:    tools_path = gcc_toolchain_path + "/" + abi_subpath + "/bin"
platform/android/detect.py:    lib_sysroot = env["ANDROID_NDK_ROOT"] + "/platforms/" + env['ndk_platform'] + "/" + env['ARCH']
platform/android/export/export.cpp:             String dst_path = "lib/" + abi + "/" + p_so.path.get_file();
platform/android/java/src/com/google/android/vending/expansion/downloader/impl/DownloadThread.java:     mUserAgent = "APKXDL (Linux; U; Android " + android.os.Build.VERSION.RELEASE + ";" + Locale.getDefault().toString() + "; " + android.os.Build.DEVICE + "/" + android.os.Build.ID + ")" +
platform/android/java/src/org/godotengine/godot/GodotIO.java:               am.open(ad.path + "/" + fname);
platform/iphone/export/export.cpp:          Error err = p_handler(current_dir + "/" + path, p_userdata);
platform/iphone/export/export.cpp:          String destination = destination_dir + "/" + asset.get_file();
platform/osx/export/export.cpp:             file = tmp_app_path_name + "/" + file;
platform/x11/os_x11.cpp:            String tested_path = path_elems[i] + "/" + message_programs[k];
scene/3d/baked_lightmap.cpp:            bake_step_function(step++, RTR("Plotting Meshes: ") + " (" + itos(pmc + 1) + "/" + itos(mesh_list.size()) + ")");
scene/3d/baked_lightmap.cpp:            bake_step_function(step++, RTR("Plotting Lights:") + " (" + itos(pmc + 1) + "/" + itos(light_list.size()) + ")");
scene/3d/baked_lightmap.cpp:            btd.text = RTR("Lighting Meshes: ") + mesh_name + " (" + itos(pmc) + "/" + itos(mesh_list.size()) + ")";
scene/3d/gi_probe.cpp:          bake_step_function(pmc, RTR("Plotting Meshes") + " " + itos(pmc) + "/" + itos(mesh_list.size()));
scene/main/node.cpp:        path = String(p->get_name()) + "/" + p->get_path_to(n);
scene/resources/packed_scene.cpp:                   WARN_PRINT(String("Node '" + String(ret_nodes[0]->get_path_to(parent)) + "/" + String(snames[n.name]) + "' was modified from inside an instance, but it has vanished.").ascii().get_data());
scene/resources/visual_shader.cpp:          prop_name += "/" + itos(E->key());
akien-mga commented 5 years ago

I think it would be good to change most of those (excluding false positives) to use plus_file() indeed.

NilsIrl commented 5 years ago

Wouldn't

initial_subname = initial_subname.plus_file(data->path[i]);

Make useless allocations?

If this is the case wouldn't it better to also add another method to String that adds without creating a new string using += rather than +

akien-mga commented 5 years ago

I guess the few += "/" occurrences can stay as is, it's probably not worth over engineering it.

NilsIrl commented 5 years ago

Working on it (so that someone else doesn't work on it at the same time)

akien-mga commented 5 years ago

Fixed by #29815.