From 470153c0df36068292a3237948b6f04fcf387849 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Fri, 15 Jul 2022 00:03:45 -0700 Subject: [PATCH] Refactor abbreviation set into its own type Previously the abbreviation map was just an unordered map; switch it to a real class so we can hang methods off of it. --- src/abbrs.cpp | 100 +++++++++++++++++++++++++++++------------- src/abbrs.h | 61 ++++++++++++++++++++------ src/builtins/abbr.cpp | 58 +++++++++--------------- src/complete.cpp | 22 ++++++---- src/env.cpp | 2 +- src/fish_tests.cpp | 27 +++++++++--- 6 files changed, 173 insertions(+), 97 deletions(-) diff --git a/src/abbrs.cpp b/src/abbrs.cpp index 435f0830a..a91d8c07f 100644 --- a/src/abbrs.cpp +++ b/src/abbrs.cpp @@ -6,57 +6,97 @@ #include "global_safety.h" #include "wcstringutil.h" -static relaxed_atomic_t k_abbrs_next_order{0}; - -abbreviation_t::abbreviation_t(wcstring replacement, abbrs_position_t position, bool from_universal) - : replacement(std::move(replacement)), +abbreviation_t::abbreviation_t(wcstring name, wcstring replacement, abbrs_position_t position, + bool from_universal) + : name(std::move(name)), + replacement(std::move(replacement)), position(position), - from_universal(from_universal), - order(++k_abbrs_next_order) {} + from_universal(from_universal) {} -acquired_lock abbrs_get_map() { - static owning_lock> abbrs; +acquired_lock abbrs_get_set() { + static owning_lock abbrs; return abbrs.acquire(); } -maybe_t abbrs_expand(const wcstring &token, abbrs_position_t position) { - auto abbrs = abbrs_get_map(); - auto iter = abbrs->find(token); - maybe_t result{}; - if (iter != abbrs->end()) { - const abbreviation_t &abbr = iter->second; - // Expand only if the positions are "compatible." - if (abbr.position == position || abbr.position == abbrs_position_t::anywhere) { - result = abbr.replacement; +maybe_t abbrs_set_t::expand(const wcstring &token, abbrs_position_t position) const { + // Later abbreviations take precedence so walk backwards. + for (auto it = abbrs_.rbegin(); it != abbrs_.rend(); ++it) { + const abbreviation_t &abbr = *it; + // Expand only if the abbreviation expands anywhere or in the given position. + if (!(abbr.position == position || abbr.position == abbrs_position_t::anywhere)) { + continue; + } + + // Expand only if the name matches. + if (token != abbr.name) { + continue; + } + + return abbr.replacement; + } + return none(); +} + +void abbrs_set_t::add(abbreviation_t &&abbr) { + assert(!abbr.name.empty() && "Invalid name"); + bool inserted = used_names_.insert(abbr.name).second; + if (!inserted) { + // Name was already used, do a linear scan to find it. + auto where = std::find_if(abbrs_.begin(), abbrs_.end(), [&](const abbreviation_t &other) { + return other.name == abbr.name; + }); + assert(where != abbrs_.end() && "Abbreviation not found though its name was present"); + abbrs_.erase(where); + } + abbrs_.push_back(std::move(abbr)); +} + +void abbrs_set_t::rename(const wcstring &old_name, const wcstring &new_name) { + bool erased = this->used_names_.erase(old_name) > 0; + bool inserted = this->used_names_.insert(new_name).second; + assert(erased && inserted && "Old name not found or new name already present"); + (void)erased; + (void)inserted; + for (auto &abbr : abbrs_) { + if (abbr.name == old_name) { + abbr.name = new_name; + break; } } - return result; } -wcstring_list_t abbrs_get_keys() { - auto abbrs = abbrs_get_map(); - wcstring_list_t keys; - keys.reserve(abbrs->size()); - for (const auto &kv : *abbrs) { - keys.push_back(kv.first); +bool abbrs_set_t::erase(const wcstring &name) { + bool erased = this->used_names_.erase(name) > 0; + if (!erased) { + return false; } - return keys; + for (auto it = abbrs_.begin(); it != abbrs_.end(); ++it) { + if (it->name == name) { + abbrs_.erase(it); + return true; + } + } + assert(false && "Unable to find named abbreviation"); + return false; } -void abbrs_import_from_uvars(const std::unordered_map &uvars) { - auto abbrs = abbrs_get_map(); +void abbrs_set_t::import_from_uvars(const std::unordered_map &uvars) { const wchar_t *const prefix = L"_fish_abbr_"; size_t prefix_len = wcslen(prefix); - wcstring name; const bool from_universal = true; for (const auto &kv : uvars) { if (string_prefixes_string(prefix, kv.first)) { wcstring escaped_name = kv.first.substr(prefix_len); + wcstring name; if (unescape_string(escaped_name, &name, unescape_flags_t{}, STRING_STYLE_VAR)) { wcstring replacement = join_strings(kv.second.as_list(), L' '); - abbrs->emplace(name, abbreviation_t{std::move(replacement), - abbrs_position_t::command, from_universal}); + this->add(abbreviation_t{std::move(name), std::move(replacement), + abbrs_position_t::command, from_universal}); } } } } + +maybe_t abbrs_expand(const wcstring &token, abbrs_position_t position) { + return abbrs_get_set()->expand(token, position); +} diff --git a/src/abbrs.h b/src/abbrs.h index 4ab06f557..ef6d89873 100644 --- a/src/abbrs.h +++ b/src/abbrs.h @@ -4,10 +4,13 @@ #define FISH_ABBRS_H #include +#include #include "common.h" #include "maybe.h" +class env_var_t; + /// Controls where in the command line abbreviations may expand. enum class abbrs_position_t : uint8_t { command, // expand in command position @@ -15,6 +18,10 @@ enum class abbrs_position_t : uint8_t { }; struct abbreviation_t { + // Abbreviation name. This is unique within the abbreviation set. + // This is used as the token to match unless we have a regex. + wcstring name{}; + // Replacement string. wcstring replacement{}; @@ -24,30 +31,56 @@ struct abbreviation_t { // Mark if we came from a universal variable. bool from_universal{}; - // A monotone key to allow reconstructing definition order. - uint64_t order{}; + // \return true if this is a regex abbreviation. + bool is_regex() const { return false; } - explicit abbreviation_t(wcstring replacement, + explicit abbreviation_t(wcstring name, wcstring replacement, abbrs_position_t position = abbrs_position_t::command, bool from_universal = false); abbreviation_t() = default; }; -using abbrs_map_t = std::unordered_map; +class abbrs_set_t { + public: + /// \return the replacement value for a abbreviation token, if any. + /// The \p position is given to describe where the token was found. + maybe_t expand(const wcstring &token, abbrs_position_t position) const; -/// \return the mutable map of abbreviations, keyed by name. -acquired_lock abbrs_get_map(); + /// Add an abbreviation. Any abbreviation with the same name is replaced. + void add(abbreviation_t &&abbr); -/// \return the replacement value for a abbreviation token, if any. + /// Rename an abbreviation. This asserts that the old name is used, and the new name is not; the + /// caller should check these beforehand with has_name(). + void rename(const wcstring &old_name, const wcstring &new_name); + + /// Erase an abbreviation by name. + /// \return true if erased, false if not found. + bool erase(const wcstring &name); + + /// \return true if we have an abbreviation with the given name. + bool has_name(const wcstring &name) const { return used_names_.count(name) > 0; } + + /// \return a reference to the abbreviation list. + const std::vector &list() const { return abbrs_; } + + /// Import from a universal variable set. + void import_from_uvars(const std::unordered_map &uvars); + + private: + /// List of abbreviations, in definition order. + std::vector abbrs_{}; + + /// Set of used abbrevation names. + /// This is to avoid a linear scan when adding new abbreviations. + std::unordered_set used_names_; +}; + +/// \return the global mutable set of abbreviations. +acquired_lock abbrs_get_set(); + +/// \return the replacement value for a abbreviation token, if any, using the global set. /// The \p position is given to describe where the token was found. maybe_t abbrs_expand(const wcstring &token, abbrs_position_t position); -/// \return the list of abbreviation keys. -wcstring_list_t abbrs_get_keys(); - -/// Import any abbreviations from universal variables. -class env_var_t; -void abbrs_import_from_uvars(const std::unordered_map &uvars); - #endif diff --git a/src/builtins/abbr.cpp b/src/builtins/abbr.cpp index cf0420f37..99c1827c5 100644 --- a/src/builtins/abbr.cpp +++ b/src/builtins/abbr.cpp @@ -73,10 +73,9 @@ struct abbr_options_t { // Print abbreviations in a fish-script friendly way. static int abbr_show(const abbr_options_t &, io_streams_t &streams) { - const auto abbrs = abbrs_get_map(); - for (const auto &kv : *abbrs) { - wcstring name = escape_string(kv.first); - const auto &abbr = kv.second; + const auto abbrs = abbrs_get_set(); + for (const auto &abbr : abbrs->list()) { + wcstring name = escape_string(abbr.name); wcstring value = escape_string(abbr.replacement); const wchar_t *scope = (abbr.from_universal ? L"-U " : L""); streams.out.append_format(L"abbr -a %ls-- %ls %ls\n", scope, name.c_str(), value.c_str()); @@ -92,9 +91,9 @@ static int abbr_list(const abbr_options_t &opts, io_streams_t &streams) { opts.args.front().c_str()); return STATUS_INVALID_ARGS; } - const auto abbrs = abbrs_get_map(); - for (const auto &kv : *abbrs) { - wcstring name = escape_string(kv.first); + const auto abbrs = abbrs_get_set(); + for (const auto &abbr : abbrs->list()) { + wcstring name = escape_string(abbr.name); name.push_back(L'\n'); streams.out.append(name); } @@ -121,38 +120,29 @@ static int abbr_rename(const abbr_options_t &opts, io_streams_t &streams) { new_name.c_str()); return STATUS_INVALID_ARGS; } - auto abbrs = abbrs_get_map(); + auto abbrs = abbrs_get_set(); - auto old_iter = abbrs->find(old_name); - if (old_iter == abbrs->end()) { + if (!abbrs->has_name(old_name)) { streams.err.append_format(_(L"%ls %ls: No abbreviation named %ls\n"), CMD, subcmd, old_name.c_str()); return STATUS_CMD_ERROR; } - - // Cannot overwrite an abbreviation. - auto new_iter = abbrs->find(new_name); - if (new_iter != abbrs->end()) { + if (abbrs->has_name(new_name)) { streams.err.append_format( _(L"%ls %ls: Abbreviation %ls already exists, cannot rename %ls\n"), CMD, subcmd, new_name.c_str(), old_name.c_str()); return STATUS_INVALID_ARGS; } - - abbreviation_t abbr = std::move(old_iter->second); - abbrs->erase(old_iter); - bool inserted = abbrs->insert(std::make_pair(std::move(new_name), std::move(abbr))).second; - assert(inserted && "Should have successfully inserted"); - (void)inserted; + abbrs->rename(old_name, new_name); return STATUS_CMD_OK; } // Test if any args is an abbreviation. static int abbr_query(const abbr_options_t &opts, io_streams_t &) { // Return success if any of our args matches an abbreviation. - const auto abbrs = abbrs_get_map(); + const auto abbrs = abbrs_get_set(); for (const auto &arg : opts.args) { - if (abbrs->find(arg) != abbrs->end()) { + if (abbrs->has_name(arg)) { return STATUS_CMD_OK; } } @@ -177,18 +167,16 @@ static int abbr_add(const abbr_options_t &opts, io_streams_t &streams) { name.c_str()); return STATUS_INVALID_ARGS; } - abbreviation_t abbr{L""}; + wcstring replacement; for (auto iter = opts.args.begin() + 1; iter != opts.args.end(); ++iter) { - if (!abbr.replacement.empty()) abbr.replacement.push_back(L' '); - abbr.replacement.append(*iter); - } - if (opts.position) { - abbr.position = *opts.position; + if (!replacement.empty()) replacement.push_back(L' '); + replacement.append(*iter); } + abbrs_position_t position = opts.position ? *opts.position : abbrs_position_t::command; + abbreviation_t abbr{name, std::move(replacement), position}; // Note historically we have allowed overwriting existing abbreviations. - auto abbrs = abbrs_get_map(); - (*abbrs)[name] = std::move(abbr); + abbrs_get_set()->add(std::move(abbr)); return STATUS_CMD_OK; } @@ -201,13 +189,10 @@ static int abbr_erase(const abbr_options_t &opts, io_streams_t &) { // Erase each. If any is not found, return ENV_NOT_FOUND which is historical. int result = STATUS_CMD_OK; - auto abbrs = abbrs_get_map(); + auto abbrs = abbrs_get_set(); for (const auto &arg : opts.args) { - auto iter = abbrs->find(arg); - if (iter == abbrs->end()) { + if (!abbrs->erase(arg)) { result = ENV_NOT_FOUND; - } else { - abbrs->erase(iter); } } return result; @@ -219,7 +204,7 @@ maybe_t builtin_abbr(parser_t &parser, io_streams_t &streams, const wchar_t const wchar_t *cmd = argv[0]; abbr_options_t opts; // Note 1 is returned by wgetopt to indicate a non-option argument. - enum { NON_OPTION_ARGUMENT = 1 }; + enum { NON_OPTION_ARGUMENT = 1, REGEX_SHORT }; // Note the leading '-' causes wgetopter to return arguments in order, instead of permuting // them. We need this behavior for compatibility with pre-builtin abbreviations where options @@ -244,7 +229,6 @@ maybe_t builtin_abbr(parser_t &parser, io_streams_t &streams, const wchar_t while ((opt = w.wgetopt_long(argc, argv, short_options, long_options, nullptr)) != -1) { switch (opt) { case NON_OPTION_ARGUMENT: - // Non-option argument. // If --add is specified (or implied by specifying no other commands), all // unrecognized options after the *second* non-option argument are considered part // of the abbreviation expansion itself, rather than options to the abbr command. diff --git a/src/complete.cpp b/src/complete.cpp index c1ef81b2e..d710fba4c 100644 --- a/src/complete.cpp +++ b/src/complete.cpp @@ -669,18 +669,24 @@ void completer_t::complete_cmd(const wcstring &str_cmd) { } void completer_t::complete_abbr(const wcstring &cmd) { - // Copy the map, so we don't hold the lock across the call to complete_strings. - abbrs_map_t abbrs = *abbrs_get_map(); + // Copy the list of names and descriptions so as not to hold the lock across the call to + // complete_strings. completion_list_t possible_comp; - possible_comp.reserve(abbrs.size()); - for (const auto &kv : abbrs) { - possible_comp.emplace_back(kv.first); + std::unordered_map descs; + { + auto abbrs = abbrs_get_set(); + for (const auto &abbr : abbrs->list()) { + if (!abbr.is_regex()) { + possible_comp.emplace_back(abbr.name); + descs[abbr.name] = abbr.replacement; + } + } } auto desc_func = [&](const wcstring &key) { - auto iter = abbrs.find(key); - assert(iter != abbrs.end() && "Abbreviation not found"); - return format_string(ABBR_DESC, iter->second.replacement.c_str()); + auto iter = descs.find(key); + assert(iter != descs.end() && "Abbreviation not found"); + return format_string(ABBR_DESC, iter->second.c_str()); }; this->complete_strings(cmd, desc_func, possible_comp, COMPLETE_NO_SPACE); } diff --git a/src/env.cpp b/src/env.cpp index 2da05949d..92bf37117 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -456,7 +456,7 @@ void env_init(const struct config_paths_t *paths, bool do_uvars, bool default_pa // Import any abbreviations from uvars. // Note we do not dynamically react to changes. - abbrs_import_from_uvars(table); + abbrs_get_set()->import_from_uvars(table); } } diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 07f79271b..114b419c1 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2467,13 +2467,12 @@ static void test_ifind_fuzzy() { static void test_abbreviations() { say(L"Testing abbreviations"); - { - auto abbrs = abbrs_get_map(); - abbrs->emplace(L"gc", L"git checkout"); - abbrs->emplace(L"foo", L"bar"); - abbrs->emplace(L"gx", L"git checkout"); - abbrs->emplace(L"yin", abbreviation_t(L"yang", abbrs_position_t::anywhere)); + auto abbrs = abbrs_get_set(); + abbrs->add(abbreviation_t(L"gc", L"git checkout")); + abbrs->add(abbreviation_t(L"foo", L"bar")); + abbrs->add(abbreviation_t(L"gx", L"git checkout")); + abbrs->add(abbreviation_t(L"yin", L"yang", abbrs_position_t::anywhere)); } auto cmd = abbrs_position_t::command; @@ -2548,6 +2547,19 @@ static void test_abbreviations() { err(L"command yin incorrectly expanded on line %ld to '%ls'", (long)__LINE__, result->c_str()); } + + // Renaming works. + { + auto abbrs = abbrs_get_set(); + do_test(!abbrs->has_name(L"gcc")); + do_test(abbrs->has_name(L"gc")); + abbrs->rename(L"gc", L"gcc"); + do_test(abbrs->has_name(L"gcc")); + do_test(!abbrs->has_name(L"gc")); + do_test(!abbrs->erase(L"gc")); + do_test(abbrs->erase(L"gcc")); + do_test(!abbrs->erase(L"gcc")); + } } /// Test path functions. @@ -3507,7 +3519,7 @@ static void test_complete() { // Test abbreviations. function_add(L"testabbrsonetwothreefour", func_props); - abbrs_get_map()->emplace(L"testabbrsonetwothreezero", L"expansion"); + abbrs_get_set()->add(abbreviation_t(L"testabbrsonetwothreezero", L"expansion")); completions = complete(L"testabbrsonetwothree", {}, parser->context()); do_test(completions.size() == 2); do_test(completions.at(0).completion == L"four"); @@ -3515,6 +3527,7 @@ static void test_complete() { // Abbreviations should not have a space after them. do_test(completions.at(1).completion == L"zero"); do_test((completions.at(1).flags & COMPLETE_NO_SPACE) != 0); + abbrs_get_set()->erase(L"testabbrsonetwothreezero"); // Test wraps. do_test(comma_join(complete_get_wrap_targets(L"wrapper1")).empty());