Cleanup common.h

Remove a bunch of headers, simplify lots of code, migrate it into .cpp files.

Debug build time improves by ~3 seconds on my Mac.

configure.ac

@@ -283,7 +283,7 @@ fi
 # Check presense of various header files
 #
 
-AC_CHECK_HEADERS([getopt.h termios.h sys/resource.h term.h ncurses/term.h ncurses.h ncurses/curses.h curses.h stropts.h siginfo.h sys/select.h sys/ioctl.h execinfo.h spawn.h sys/sysctl.h])
+AC_CHECK_HEADERS([getopt.h termios.h sys/resource.h term.h ncurses/term.h ncurses.h ncurses/curses.h curses.h stropts.h siginfo.h sys/select.h sys/ioctl.h execinfo.h spawn.h sys/sysctl.h xlocale.h])
 
 if test x$local_gettext != xno; then
   AC_CHECK_HEADERS([libintl.h])

src/builtin_math.cpp

@@ -3,6 +3,7 @@
 
 #include <errno.h>
 #include <stddef.h>
+#include <string.h>
 
 #include <algorithm>
 #include <cmath>

src/builtin_pwd.cpp

@@ -1,6 +1,8 @@
 // Implementation of the pwd builtin.
 #include "config.h"  // IWYU pragma: keep
 
+#include <string.h>
+
 #include "builtin.h"
 #include "builtin_pwd.h"
 #include "common.h"

src/builtin_realpath.cpp

@@ -3,6 +3,7 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 #include <wchar.h>
 
 #include "builtin.h"

src/builtin_test.cpp

@@ -5,10 +5,12 @@
 
 #include <errno.h>
 #include <stdarg.h>
+#include <string.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
 #include <wchar.h>
+#include <wctype.h>
 
 #include <cmath>
 #include <memory>

src/common.cpp

@@ -38,6 +38,7 @@
 
 #include <algorithm>
 #include <atomic>
+#include <locale>
 #include <memory>  // IWYU pragma: keep
 #include <type_traits>
 
@@ -692,56 +693,52 @@ void debug_safe(int level, const char *msg, const char *param1, const char *para
     errno = errno_old;
 }
 
-void format_long_safe(char buff[64], long val) {
+// Careful to not negate LLONG_MIN.
-    if (val == 0) {
+static unsigned long long absolute_value(long long x) {
-        strcpy(buff, "0");
+    if (x >= 0) return static_cast<unsigned long long>(x);
-    } else {
+    x = -(x + 1);
-        // Generate the string in reverse.
+    return static_cast<unsigned long long>(x) + 1;
-        size_t idx = 0;
+}
-        bool negative = (val < 0);
 
-        // Note that we can't just negate val if it's negative, because it may be the most negative
+template <typename CharT>
-        // value. We do rely on round-towards-zero division though.
+void format_safe_impl(CharT *buff, size_t size, unsigned long long val) {
+    size_t idx = 0;
+    if (val == 0) {
+        buff[idx++] = '0';
+    } else {
+        // Generate the string backwards, then reverse it.
         while (val != 0) {
-            long rem = val % 10;
+            buff[idx++] = (val % 10) + '0';
-            buff[idx++] = '0' + (rem < 0 ? -rem : rem);
             val /= 10;
         }
-        if (negative) buff[idx++] = '-';
+        std::reverse(buff, buff + idx);
-        buff[idx] = 0;
-
-        size_t left = 0, right = idx - 1;
-        while (left < right) {
-            char tmp = buff[left];
-            buff[left++] = buff[right];
-            buff[right--] = tmp;
     }
+    buff[idx++] = '\0';
+    assert(idx <= size && "Buffer overflowed");
+}
+
+void format_long_safe(char buff[64], long val) {
+    unsigned long long uval = absolute_value(val);
+    if (val >= 0) {
+        format_safe_impl(buff, 64, uval);
+    } else {
+        buff[0] = '-';
+        format_safe_impl(buff + 1, 63, uval);
     }
 }
 
 void format_long_safe(wchar_t buff[64], long val) {
-    if (val == 0) {
+    unsigned long long uval = absolute_value(val);
-        wcscpy(buff, L"0");
+    if (val >= 0) {
+        format_safe_impl(buff, 64, uval);
     } else {
-        // Generate the string in reverse.
+        buff[0] = '-';
-        size_t idx = 0;
+        format_safe_impl(buff + 1, 63, uval);
-        bool negative = (val < 0);
+    }
+}
 
-        while (val != 0) {
+void format_ullong_safe(wchar_t buff[64], unsigned long long val) {
-            long rem = val % 10;
+    return format_safe_impl(buff, 64, val);
-            buff[idx++] = L'0' + (wchar_t)(rem < 0 ? -rem : rem);
-            val /= 10;
-        }
-        if (negative) buff[idx++] = L'-';
-        buff[idx] = 0;
-
-        size_t left = 0, right = idx - 1;
-        while (left < right) {
-            wchar_t tmp = buff[left];
-            buff[left++] = buff[right];
-            buff[right--] = tmp;
-        }
-    }
 }
 
 void narrow_string_safe(char buff[64], const wchar_t *s) {
@@ -1956,6 +1953,36 @@ int string_fuzzy_match_t::compare(const string_fuzzy_match_t &rhs) const {
     return 0;  // equal
 }
 
+template <bool Fuzzy, typename T>
+size_t ifind_impl(const T &haystack, const T &needle) {
+    using char_t = typename T::value_type;
+    std::locale locale;
+
+    auto ieq = [&locale](char_t c1, char_t c2) {
+        if (c1 == c2 || std::toupper(c1, locale) == std::toupper(c2, locale)) return true;
+
+        // In fuzzy matching treat treat `-` and `_` as equal (#3584).
+        if (Fuzzy) {
+            if ((c1 == '-' || c1 == '_') && (c2 == '-' || c2 == '_')) return true;
+        }
+        return false;
+    };
+
+    auto result = std::search(haystack.begin(), haystack.end(), needle.begin(), needle.end(), ieq);
+    if (result != haystack.end()) {
+        return result - haystack.begin();
+    }
+    return T::npos;
+}
+
+size_t ifind(const wcstring &haystack, const wcstring &needle, bool fuzzy) {
+    return fuzzy ? ifind_impl<true>(haystack, needle) : ifind_impl<false>(haystack, needle);
+}
+
+size_t ifind(const std::string &haystack, const std::string &needle, bool fuzzy) {
+    return fuzzy ? ifind_impl<true>(haystack, needle) : ifind_impl<false>(haystack, needle);
+}
+
 wcstring_list_t split_string(const wcstring &val, wchar_t sep) {
     wcstring_list_t out;
     size_t pos = 0, end = val.size();

src/common.h

@@ -5,33 +5,19 @@
 
 #include <errno.h>
 #include <limits.h>
-#include <pthread.h>
-#include <stdarg.h>  // IWYU pragma: keep
-#include <stddef.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <termios.h>
-#include <wchar.h>
 #ifdef HAVE_SYS_IOCTL_H
 #include <sys/ioctl.h>  // IWYU pragma: keep
 #endif
 
 #include <algorithm>
 #include <functional>
-#include <iterator>
 #include <memory>
 #include <mutex>
-#include <sstream>
 #include <string>
-#include <tuple>
-#include <type_traits>
-#include <unordered_map>
 #include <vector>
 
 #include "fallback.h"  // IWYU pragma: keep
 #include "maybe.h"
-#include "signal.h"  // IWYU pragma: keep
 
 // Define a symbol we can use elsewhere in our code to determine if we're being built on MS Windows
 // under Cygwin.
@@ -361,61 +347,13 @@ bool string_suffixes_string_case_insensitive(const wcstring &proposed_suffix,
 bool string_prefixes_string_case_insensitive(const wcstring &proposed_prefix,
                                              const wcstring &value);
 
-/// Case-insensitive string search, templated for use with both std::string and std::wstring.
+/// Case-insensitive string search, modeled after std::string::find().
-/// Modeled after std::string::find().
 /// \param fuzzy indicates this is being used for fuzzy matching and case insensitivity is
 /// expanded to include symbolic characters (#3584).
 /// \return the offset of the first case-insensitive matching instance of `needle` within
 /// `haystack`, or `string::npos()` if no results were found.
-template <typename T>
+size_t ifind(const wcstring &haystack, const wcstring &needle, bool fuzzy = false);
-size_t ifind(const T &haystack, const T &needle, bool fuzzy = false) {
+size_t ifind(const std::string &haystack, const std::string &needle, bool fuzzy = false);
-    using char_t = typename T::value_type;
-    auto locale = std::locale();
-
-    std::function<bool(char_t, char_t)> icase_eq;
-
-    if (!fuzzy) {
-        icase_eq = [&locale](char_t c1, char_t c2) {
-            return std::toupper(c1, locale) == std::toupper(c2, locale);
-        };
-    } else {
-        icase_eq = [&locale](char_t c1, char_t c2) {
-            // This `ifind()` call is being used for fuzzy string matching. Further extend case
-            // insensitivity to treat `-` and `_` as equal (#3584).
-
-            // The two lines below were tested to be 27% faster than
-            //      (c1 == '_' || c1 == '-') && (c2 == '-' || c2 == '_')
-            // while returning no false positives for all (c1, c2) combinations in the printable
-            // range (0x20-0x7E). It might return false positives outside that range, but fuzzy
-            // comparisons are typically called for file names only, which are unlikely to have
-            // such characters and this entire function is 100% broken on unicode so there's no
-            // point in worrying about anything outside of the ANSII range.
-            // ((c1 == Literal<char_t>('_') || c1 == Literal<char_t>('-')) &&
-            // ((c1 ^ c2) == (Literal<char_t>('-') ^ Literal<char_t>('_'))));
-
-            // One of the following would be an illegal comparison between a char and a wchar_t.
-            // However, placing them behind a constexpr gate results in the elision of the if
-            // statement and the incorrect branch, with the compiler's SFINAE support suppressing
-            // any errors in the branch not taken.
-            if (sizeof(char_t) == sizeof(char)) {
-                return std::toupper(c1, locale) == std::toupper(c2, locale) ||
-                ((c1 == '_' || c1 == '-') &&
-                ((c1 ^ c2) == ('-' ^ '_')));
-            } else {
-                return std::toupper(c1, locale) == std::toupper(c2, locale) ||
-                ((c1 == L'_' || c1 == L'-') &&
-                ((c1 ^ c2) == (L'-' ^ L'_')));
-            }
-        };
-    }
-
-    auto result =
-        std::search(haystack.begin(), haystack.end(), needle.begin(), needle.end(), icase_eq);
-    if (result != haystack.end()) {
-        return result - haystack.begin();
-    }
-    return T::npos;
-}
 
 /// Split a string by a separator character.
 wcstring_list_t split_string(const wcstring &val, wchar_t sep);
@@ -565,55 +503,40 @@ void debug_safe(int level, const char *msg, const char *param1 = NULL, const cha
 /// Writes out a long safely.
 void format_long_safe(char buff[64], long val);
 void format_long_safe(wchar_t buff[64], long val);
+void format_ullong_safe(wchar_t buff[64], unsigned long long val);
 
 /// "Narrows" a wide character string. This just grabs any ASCII characters and trunactes.
 void narrow_string_safe(char buff[64], const wchar_t *s);
 
-template <typename T>
+inline wcstring to_string(long x) {
-T from_string(const wcstring &x) {
+    wchar_t buff[64];
-    T result;
-    std::wstringstream stream(x);
-    stream >> result;
-    return result;
-}
-
-template <typename T>
-T from_string(const std::string &x) {
-    T result = T();
-    std::stringstream stream(x);
-    stream >> result;
-    return result;
-}
-
-template <typename T>
-wcstring to_string(const T &x) {
-    std::wstringstream stream;
-    stream << x;
-    return stream.str();
-}
-
-// wstringstream is a huge memory pig. Let's provide some specializations where we can.
-template <>
-inline wcstring to_string(const long &x) {
-    wchar_t buff[128];
     format_long_safe(buff, x);
     return wcstring(buff);
 }
 
-template <>
+inline wcstring to_string(int x) { return to_string(static_cast<long>(x)); }
-inline bool from_string(const std::string &x) {
+
-    return !x.empty() && strchr("YTyt1", x.at(0));
+inline wcstring to_string(size_t x) {
+    wchar_t buff[64];
+    format_ullong_safe(buff, x);
+    return wcstring(buff);
 }
 
-template <>
+inline bool bool_from_string(const std::string &x) {
-inline bool from_string(const wcstring &x) {
+    if (x.empty()) return false;
-    return !x.empty() && wcschr(L"YTyt1", x.at(0));
+    switch (x.front()) {
+        case 'Y':
+        case 'T':
+        case 'y':
+        case 't':
+        case '1':
+            return true;
+        default:
+            return false;
+    }
 }
 
-template <>
+inline bool bool_from_string(const wcstring &x) { return !x.empty() && wcschr(L"YTyt1", x.at(0)); }
-inline wcstring to_string(const int &x) {
-    return to_string(static_cast<long>(x));
-}
 
 wchar_t **make_null_terminated_array(const wcstring_list_t &lst);
 char **make_null_terminated_array(const std::vector<std::string> &lst);
@@ -1022,22 +945,6 @@ static const wchar_t *enum_to_str(T enum_val, const enum_map<T> map[]) {
     return NULL;
 };
 
-template <typename... Args>
-using tuple_list = std::vector<std::tuple<Args...>>;
-
-// Given a container mapping one X to many Y, return a list of {X,Y}
-template <typename X, typename Y>
-inline tuple_list<X, Y> flatten(const std::unordered_map<X, std::vector<Y>> &list) {
-    tuple_list<X, Y> results(list.size() * 1.5);  // just a guess as to the initial size
-    for (auto &kv : list) {
-        for (auto &v : kv.second) {
-            results.emplace_back(std::make_tuple(kv.first, v));
-        }
-    }
-
-    return results;
-}
-
 void redirect_tty_output();
 
 // Minimum allowed terminal size and default size if the detected size is not reasonable.
@@ -1123,7 +1030,7 @@ private:
     const std::function<void()> cleanup;
 public:
     cleanup_t(std::function<void()> exit_actions)
-        : cleanup{exit_actions} {}
+        : cleanup{std::move(exit_actions)} {}
     ~cleanup_t() {
         cleanup();
     }

src/complete.cpp

@@ -171,6 +171,10 @@ struct equal_to<completion_entry_t> {
 typedef std::unordered_set<completion_entry_t> completion_entry_set_t;
 static owning_lock<completion_entry_set_t> s_completion_set;
 
+/// Completion "wrapper" support. The map goes from wrapping-command to wrapped-command-list.
+using wrapper_map_t = std::unordered_map<wcstring, wcstring_list_t>;
+static owning_lock<wrapper_map_t> wrapper_map;
+
 /// Comparison function to sort completions by their order field.
 static bool compare_completions_by_order(const completion_entry_t &p1,
                                          const completion_entry_t &p2) {
@@ -1637,22 +1641,20 @@ wcstring complete_print() {
         }
     }
 
-    // Append wraps. This is a wonky interface where even values are the commands, and odd values
+    // Append wraps.
-    // are the targets that they wrap.
+    auto locked_wrappers = wrapper_map.acquire();
-    auto wrap_pairs = complete_get_wrap_pairs();
+    for (const auto &entry : *locked_wrappers) {
-    for (const auto &entry : wrap_pairs) {
+        const wcstring &src = entry.first;
-        append_format(out, L"complete --command %ls --wraps %ls\n", std::get<0>(entry).c_str(),
+        for (const wcstring &target : entry.second) {
-                      std::get<1>(entry).c_str());
+            append_format(out, L"complete --command %ls --wraps %ls\n", src.c_str(),
+                          target.c_str());
+        }
     }
     return out;
 }
 
 void complete_invalidate_path() { completion_autoloader.invalidate(); }
 
-/// Completion "wrapper" support. The map goes from wrapping-command to wrapped-command-list.
-using wrapper_map_t = std::unordered_map<wcstring, wcstring_list_t>;
-static owning_lock<wrapper_map_t> wrapper_map;
-
 /// Add a new target that wraps a command. Example: __fish_XYZ (function) wraps XYZ (target).
 bool complete_add_wrapper(const wcstring &command, const wcstring &new_target) {
     if (command.empty() || new_target.empty()) {
@@ -1704,8 +1706,3 @@ wcstring_list_t complete_get_wrap_targets(const wcstring &command) {
     if (iter == wraps.end()) return {};
     return iter->second;
 }
-
-tuple_list<wcstring, wcstring> complete_get_wrap_pairs() {
-    auto locked_map = wrapper_map.acquire();
-    return flatten(*locked_map);
-}

src/complete.h

@@ -204,8 +204,6 @@ bool complete_remove_wrapper(const wcstring &command, const wcstring &wrap_targe
 /// Returns a list of wrap targets for a given command.
 wcstring_list_t complete_get_wrap_targets(const wcstring &command);
 
-tuple_list<wcstring, wcstring> complete_get_wrap_pairs();
-
 // Observes that fish_complete_path has changed.
 void complete_invalidate_path();
 

src/env.cpp

@@ -469,7 +469,7 @@ static void update_fish_color_support(const environment_t &vars) {
     wcstring term = term_var.missing_or_empty() ? L"" : term_var->as_string();
     bool support_term256 = false;  // default to no support
     if (!fish_term256.missing_or_empty()) {
-        support_term256 = from_string<bool>(fish_term256->as_string());
+        support_term256 = bool_from_string(fish_term256->as_string());
         debug(2, L"256 color support determined by 'fish_term256'");
     } else if (term.find(L"256color") != wcstring::npos) {
         // TERM=*256color*: Explicitly supported.
@@ -501,7 +501,7 @@ static void update_fish_color_support(const environment_t &vars) {
     auto fish_term24bit = vars.get(L"fish_term24bit");
     bool support_term24bit;
     if (!fish_term24bit.missing_or_empty()) {
-        support_term24bit = from_string<bool>(fish_term24bit->as_string());
+        support_term24bit = bool_from_string(fish_term24bit->as_string());
         debug(2, L"'fish_term24bit' preference: 24-bit color %s",
               support_term24bit ? L"enabled" : L"disabled");
     } else {
@@ -956,7 +956,7 @@ void env_init(const struct config_paths_t *paths /* or NULL */) {
     vars.set_one(L"FISH_VERSION", ENV_GLOBAL, version);
 
     // Set the $fish_pid variable.
-    vars.set_one(L"fish_pid", ENV_GLOBAL, to_string<long>(getpid()));
+    vars.set_one(L"fish_pid", ENV_GLOBAL, to_string(getpid()));
 
     // Set the $hostname variable
     wcstring hostname = L"fish";
@@ -971,7 +971,7 @@ void env_init(const struct config_paths_t *paths /* or NULL */) {
         // TODO: Figure out how to handle invalid numbers better. Shouldn't we issue a diagnostic?
         long shlvl_i = fish_wcstol(str2wcstring(shlvl_var).c_str(), &end);
         if (!errno && shlvl_i >= 0) {
-            nshlvl_str = to_string<long>(shlvl_i + 1);
+            nshlvl_str = to_string(shlvl_i + 1);
         }
     }
     vars.set_one(L"SHLVL", ENV_GLOBAL | ENV_EXPORT, nshlvl_str);
@@ -1028,7 +1028,7 @@ void env_init(const struct config_paths_t *paths /* or NULL */) {
     // Set g_use_posix_spawn. Default to true.
     auto use_posix_spawn = vars.get(L"fish_use_posix_spawn");
     g_use_posix_spawn =
-        use_posix_spawn.missing_or_empty() ? true : from_string<bool>(use_posix_spawn->as_string());
+        use_posix_spawn.missing_or_empty() ? true : bool_from_string(use_posix_spawn->as_string());
 
     // Set fish_bind_mode to "default".
     vars.set_one(FISH_BIND_MODE_VAR, ENV_GLOBAL, DEFAULT_BIND_MODE);

src/exec.cpp

@@ -335,7 +335,7 @@ void internal_exec(env_stack_t &vars, job_t *j, const io_chain_t &all_ios) {
         if (shlvl_var) {
             long shlvl = fish_wcstol(shlvl_var->as_string().c_str());
             if (!errno && shlvl > 0) {
-                shlvl_str = to_string<long>(shlvl - 1);
+                shlvl_str = to_string(shlvl - 1);
             }
         }
         vars.set_one(L"SHLVL", ENV_GLOBAL | ENV_EXPORT, std::move(shlvl_str));

src/expand.cpp

@@ -787,7 +787,7 @@ static void expand_home_directory(wcstring &input, const environment_t &vars) {
 /// Expand the %self escape. Note this can only come at the beginning of the string.
 static void expand_percent_self(wcstring &input) {
     if (!input.empty() && input.front() == PROCESS_EXPAND_SELF) {
-        input.replace(0, 1, to_string<long>(getpid()));
+        input.replace(0, 1, to_string(getpid()));
     }
 }
 

src/fallback.h

@@ -3,10 +3,6 @@
 
 #include "config.h"
 
-// IWYU likes to recommend adding <term.h> when we want <ncurses.h>. If we add <term.h> it breaks
-// compiling several modules that include this header because they use symbols which are defined as
-// macros in <term.h>.
-// IWYU pragma: no_include <term.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <unistd.h>

src/fish.cpp

@@ -442,7 +442,7 @@ int main(int argc, char **argv) {
     proc_fire_event(L"PROCESS_EXIT", EVENT_EXIT, getpid(), exit_status);
 
     // Trigger any exit handlers.
-    wcstring_list_t event_args = {to_string<int>(exit_status)};
+    wcstring_list_t event_args = {to_string(exit_status)};
     event_fire_generic(L"fish_exit", &event_args);
 
     restore_term_mode();

src/maybe.h

@@ -2,7 +2,6 @@
 #define FISH_MAYBE_H
 
 #include <cassert>
-#include <utility>
 
 // A none_t is a helper type used to implicitly initialize maybe_t.
 // Example usage:

src/proc.cpp

@@ -625,8 +625,8 @@ void proc_fire_event(const wchar_t *msg, int type, pid_t pid, int status) {
     event.param1.pid = pid;
 
     event.arguments.push_back(msg);
-    event.arguments.push_back(to_string<int>(pid));
+    event.arguments.push_back(to_string(pid));
-    event.arguments.push_back(to_string<int>(status));
+    event.arguments.push_back(to_string(status));
     event_fire(&event);
     event.arguments.resize(0);
 }