From 4c08cbd050de7488adfcad000ef2b1b61f26be7c Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Wed, 10 Oct 2018 17:41:17 -0700 Subject: [PATCH 1/2] Mark completion move ctor as noexcept Move constructors aren't used unless we mark this ctor as noexcept. --- src/complete.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/complete.h b/src/complete.h index 05c946e71..db9b650b8 100644 --- a/src/complete.h +++ b/src/complete.h @@ -78,8 +78,9 @@ class completion_t { completion_t(const completion_t &); completion_t &operator=(const completion_t &); - completion_t(completion_t &&); - completion_t &operator=(completion_t &&); + // noexcepts are required for push_back to use the move ctor. + completion_t(completion_t &&) noexcept; + completion_t &operator=(completion_t &&) noexcept; // Compare two completions. No operating overlaoding to make this always explicit (there's // potentially multiple ways to compare completions). From 90d89a3262ebfe1582e294a4c7fb079d1e94bc83 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Wed, 10 Oct 2018 17:38:38 -0700 Subject: [PATCH 2/2] Use more move constructors in expansion Reduce allocations by switching to move semantics. clang-tidy detects some use-after-moves. --- src/complete.cpp | 19 ++++++++-------- src/expand.cpp | 58 +++++++++++++++++++++++++----------------------- src/expand.h | 2 +- 3 files changed, 41 insertions(+), 38 deletions(-) diff --git a/src/complete.cpp b/src/complete.cpp index 5ab485fbf..be995e652 100644 --- a/src/complete.cpp +++ b/src/complete.cpp @@ -381,8 +381,9 @@ static autoload_t completion_autoloader(L"fish_complete_path", autoloaded_comple /// Create a new completion entry. void append_completion(std::vector *completions, wcstring comp, wcstring desc, complete_flags_t flags, string_fuzzy_match_t match) { - completions->emplace_back(std::move(comp), std::move(desc), match, - resolve_auto_space(comp, flags)); + complete_flags_t resolved_flags = resolve_auto_space(comp, flags); + completion_t completion{std::move(comp), std::move(desc), match, resolved_flags}; + completions->push_back(std::move(completion)); } /// Test if the specified script returns zero. The result is cached, so that if multiple completions @@ -677,9 +678,9 @@ void completer_t::complete_cmd(const wcstring &str_cmd, bool use_function, bool if (str_cmd.find(L'/') == wcstring::npos && str_cmd.at(0) != L'~') { if (use_function) { wcstring_list_t names = function_get_names(str_cmd.at(0) == L'_'); - for (size_t i = 0; i < names.size(); i++) { + for (wcstring &name : names) { // Append all known matching functions - append_completion(&possible_comp, names.at(i)); + append_completion(&possible_comp, std::move(name)); } this->complete_strings(str_cmd, 0, &complete_function_desc, possible_comp, 0); @@ -992,9 +993,9 @@ bool completer_t::complete_param(const wcstring &scmd_orig, const wcstring &spop // Check if the short style option matches. if (short_ok(str, &o, options)) { // It's a match. - const wcstring desc = o.localized_desc(); + wcstring desc = o.localized_desc(); // Append a short-style option - append_completion(&this->completions, o.option, desc, 0); + append_completion(&this->completions, o.option, std::move(desc), 0); } // Check if the long style option matches. @@ -1035,11 +1036,11 @@ bool completer_t::complete_param(const wcstring &scmd_orig, const wcstring &spop // functions. wcstring completion = format_string(L"%ls=", whole_opt.c_str() + offset); // Append a long-style option with a mandatory trailing equal sign - append_completion(&this->completions, completion, C_(o.desc), flags); + append_completion(&this->completions, std::move(completion), C_(o.desc), flags); } // Append a long-style option - append_completion(&this->completions, whole_opt.c_str() + offset, C_(o.desc), flags); + append_completion(&this->completions, whole_opt.substr(offset), C_(o.desc), flags); } } @@ -1147,7 +1148,7 @@ bool completer_t::complete_variable(const wcstring &str, size_t start_offset) { } // Append matching environment variables - append_completion(&this->completions, comp, desc, flags, match); + append_completion(&this->completions, std::move(comp), desc, flags, match); res = true; } diff --git a/src/expand.cpp b/src/expand.cpp index 805a94cc2..ed6532202 100644 --- a/src/expand.cpp +++ b/src/expand.cpp @@ -290,14 +290,14 @@ static size_t parse_slice(const wchar_t *in, wchar_t **end_ptr, std::vector *out, size_t last_idx, +static bool expand_variables(wcstring instr, std::vector *out, size_t last_idx, parse_error_list_t *errors) { const size_t insize = instr.size(); // last_idx may be 1 past the end of the string, but no further. assert(last_idx <= insize && "Invalid last_idx"); if (last_idx == 0) { - append_completion(out, instr); + append_completion(out, std::move(instr)); return true; } @@ -313,7 +313,7 @@ static bool expand_variables(const wcstring &instr, std::vector *o } if (varexp_char_idx >= instr.size()) { // No variable expand char, we're done. - append_completion(out, instr); + append_completion(out, std::move(instr)); return true; } @@ -405,7 +405,7 @@ static bool expand_variables(const wcstring &instr, std::vector *o res.push_back(VARIABLE_EXPAND_EMPTY); } res.append(instr, var_name_and_slice_stop, wcstring::npos); - return expand_variables(res, out, varexp_char_idx, errors); + return expand_variables(std::move(res), out, varexp_char_idx, errors); } } @@ -466,7 +466,7 @@ static bool expand_variables(const wcstring &instr, std::vector *o res.pop_back(); } res.append(instr, var_name_and_slice_stop, wcstring::npos); - return expand_variables(res, out, varexp_char_idx, errors); + return expand_variables(std::move(res), out, varexp_char_idx, errors); } else { // Normal cartesian-product expansion. for (const wcstring &item : var_item_list) { @@ -483,7 +483,7 @@ static bool expand_variables(const wcstring &instr, std::vector *o } new_in.append(item); new_in.append(instr, var_name_and_slice_stop, wcstring::npos); - if (!expand_variables(new_in, out, varexp_char_idx, errors)) { + if (!expand_variables(std::move(new_in), out, varexp_char_idx, errors)) { return false; } } @@ -881,31 +881,31 @@ static void remove_internal_separator(wcstring *str, bool conv) { /// A stage in string expansion is represented as a function that takes an input and returns a list /// of output (by reference). We get flags and errors. It may return an error; if so expansion /// halts. -typedef expand_error_t (*expand_stage_t)(const wcstring &input, //!OCLINT(unused param) +typedef expand_error_t (*expand_stage_t)(wcstring input, //!OCLINT(unused param) std::vector *out, //!OCLINT(unused param) expand_flags_t flags, //!OCLINT(unused param) parse_error_list_t *errors); //!OCLINT(unused param) -static expand_error_t expand_stage_cmdsubst(const wcstring &input, std::vector *out, +static expand_error_t expand_stage_cmdsubst(wcstring input, std::vector *out, expand_flags_t flags, parse_error_list_t *errors) { if (EXPAND_SKIP_CMDSUBST & flags) { wchar_t *begin, *end; if (parse_util_locate_cmdsubst(input.c_str(), &begin, &end, true) == 0) { - append_completion(out, input); + append_completion(out, std::move(input)); } else { append_cmdsub_error(errors, SOURCE_LOCATION_UNKNOWN, L"Command substitutions not allowed"); return EXPAND_ERROR; } } else { - bool cmdsubst_ok = expand_cmdsubst(input, out, errors); + bool cmdsubst_ok = expand_cmdsubst(std::move(input), out, errors); if (!cmdsubst_ok) return EXPAND_ERROR; } return EXPAND_OK; } -static expand_error_t expand_stage_variables(const wcstring &input, std::vector *out, +static expand_error_t expand_stage_variables(wcstring input, std::vector *out, expand_flags_t flags, parse_error_list_t *errors) { // We accept incomplete strings here, since complete uses expand_string to expand incomplete // strings from the commandline. @@ -918,37 +918,36 @@ static expand_error_t expand_stage_variables(const wcstring &input, std::vector< next[i] = L'$'; } } - append_completion(out, next); + append_completion(out, std::move(next)); } else { - if (!expand_variables(next, out, next.size(), errors)) { + size_t size = next.size(); + if (!expand_variables(std::move(next), out, size, errors)) { return EXPAND_ERROR; } } return EXPAND_OK; } -static expand_error_t expand_stage_braces(const wcstring &input, std::vector *out, +static expand_error_t expand_stage_braces(wcstring input, std::vector *out, expand_flags_t flags, parse_error_list_t *errors) { return expand_braces(input, flags, out, errors); } -static expand_error_t expand_stage_home(const wcstring &input, std::vector *out, +static expand_error_t expand_stage_home(wcstring input, std::vector *out, expand_flags_t flags, parse_error_list_t *errors) { (void)errors; - wcstring next = input; - if (!(EXPAND_SKIP_HOME_DIRECTORIES & flags)) { - expand_home_directory(next); + expand_home_directory(input); } - append_completion(out, next); + append_completion(out, std::move(input)); return EXPAND_OK; } -static expand_error_t expand_stage_wildcards(const wcstring &input, std::vector *out, - expand_flags_t flags, parse_error_list_t *errors) { +static expand_error_t expand_stage_wildcards(wcstring path_to_expand, + std::vector *out, expand_flags_t flags, + parse_error_list_t *errors) { UNUSED(errors); expand_error_t result = EXPAND_OK; - wcstring path_to_expand = input; remove_internal_separator(&path_to_expand, flags & EXPAND_SKIP_WILDCARDS); const bool has_wildcard = wildcard_has(path_to_expand, true /* internal, i.e. ANY_STRING */); @@ -1031,17 +1030,17 @@ static expand_error_t expand_stage_wildcards(const wcstring &input, std::vector< // to mean don't do file expansions, so if we're not doing file expansions, just drop this // completion on the floor. if (!(flags & EXPAND_FOR_COMPLETIONS)) { - append_completion(out, path_to_expand); + append_completion(out, std::move(path_to_expand)); } } return result; } -expand_error_t expand_string(const wcstring &input, std::vector *out_completions, +expand_error_t expand_string(wcstring input, std::vector *out_completions, expand_flags_t flags, parse_error_list_t *errors) { // Early out. If we're not completing, and there's no magic in the input, we're done. if (!(flags & EXPAND_FOR_COMPLETIONS) && expand_is_clean(input)) { - append_completion(out_completions, input); + append_completion(out_completions, std::move(input)); return EXPAND_OK; } @@ -1058,8 +1057,9 @@ expand_error_t expand_string(const wcstring &input, std::vector *o for (size_t stage_idx = 0; total_result != EXPAND_ERROR && stage_idx < sizeof stages / sizeof *stages; stage_idx++) { for (size_t i = 0; total_result != EXPAND_ERROR && i < completions.size(); i++) { - const wcstring &next = completions.at(i).completion; - expand_error_t this_result = stages[stage_idx](next, &output_storage, flags, errors); + wcstring &next = completions.at(i).completion; + expand_error_t this_result = + stages[stage_idx](std::move(next), &output_storage, flags, errors); // If this_result was no match, but total_result is that we have a match, then don't // change it. if (!(this_result == EXPAND_WILDCARD_NO_MATCH && @@ -1078,7 +1078,9 @@ expand_error_t expand_string(const wcstring &input, std::vector *o if (!(flags & EXPAND_SKIP_HOME_DIRECTORIES)) { unexpand_tildes(input, &completions); } - out_completions->insert(out_completions->end(), completions.begin(), completions.end()); + out_completions->insert(out_completions->end(), + std::make_move_iterator(completions.begin()), + std::make_move_iterator(completions.end())); } return total_result; } diff --git a/src/expand.h b/src/expand.h index 6cf87645f..da11a03e0 100644 --- a/src/expand.h +++ b/src/expand.h @@ -110,7 +110,7 @@ enum expand_error_t { /// \return One of EXPAND_OK, EXPAND_ERROR, EXPAND_WILDCARD_MATCH and EXPAND_WILDCARD_NO_MATCH. /// EXPAND_WILDCARD_NO_MATCH and EXPAND_WILDCARD_MATCH are normal exit conditions used only on /// strings containing wildcards to tell if the wildcard produced any matches. -__warn_unused expand_error_t expand_string(const wcstring &input, std::vector *output, +__warn_unused expand_error_t expand_string(wcstring input, std::vector *output, expand_flags_t flags, parse_error_list_t *errors); /// expand_one is identical to expand_string, except it will fail if in expands to more than one