cleanup env code and contains()

Switch from null terminated arrays to `wcstring_list_t` for lists of
special env var names. Rename `list_contains_string` to `contains` and
modify the latter interface to not rely on a `#define`.

Rename `list_contains_string()` to `contains()` and eliminate the
current variadic implementation. Update all callers of the removed
version to use the string list version.

src/builtin.cpp

@@ -3605,10 +3605,11 @@ void builtin_destroy() {}
 bool builtin_exists(const wcstring &cmd) { return static_cast<bool>(builtin_lookup(cmd)); }
 
 /// If builtin takes care of printing help itself
+static const wcstring_list_t help_builtins({L"for", L"while", L"function", L"if", L"end", L"switch",
+                                            L"case", L"count", L"printf"});
 static bool builtin_handles_help(const wchar_t *cmd) {
     CHECK(cmd, 0);
-    return contains(cmd, L"for", L"while", L"function", L"if", L"end", L"switch", L"case", L"count",
+    return contains(help_builtins, cmd);
-                    L"printf");
 }
 
 /// Execute a builtin command

src/builtin_set.cpp

@@ -40,7 +40,8 @@ class parser_t;
     L"%ls: The number of variable indexes does not match the number of values\n"
 
 // Test if the specified variable should be subject to path validation.
-static int is_path_variable(const wchar_t *env) { return contains(env, L"PATH", L"CDPATH"); }
+static const wcstring_list_t path_variables({L"PATH", L"CDPATH"});
+static int is_path_variable(const wchar_t *env) { return contains(path_variables, env); }
 
 /// Call env_set. If this is a path variable, e.g. PATH, validate the elements. On error, print a
 /// description of the problem to stderr.
@@ -71,7 +72,7 @@ static int my_env_set(const wchar_t *key, const wcstring_list_t &val, int scope,
 
         for (i = 0; i < val.size(); i++) {
             const wcstring &dir = val.at(i);
-            if (!string_prefixes_string(L"/", dir) || list_contains_string(existing_values, dir)) {
+            if (!string_prefixes_string(L"/", dir) || contains(existing_values, dir)) {
                 any_success = true;
                 continue;
             }

src/common.cpp

@@ -459,45 +459,6 @@ void fish_setlocale() {
     omitted_newline_char = can_be_encoded(L'\x23CE') ? L'\x23CE' : L'~';
 }
 
-bool contains_internal(const wchar_t *a, int vararg_handle, ...) {
-    const wchar_t *arg;
-    va_list va;
-    bool res = false;
-
-    CHECK(a, 0);
-
-    va_start(va, vararg_handle);
-    while ((arg = va_arg(va, const wchar_t *)) != 0) {
-        if (wcscmp(a, arg) == 0) {
-            res = true;
-            break;
-        }
-    }
-    va_end(va);
-    return res;
-}
-
-/// wcstring variant of contains_internal. The first parameter is a wcstring, the rest are const
-/// wchar_t *. vararg_handle exists only to give us a POD-value to pass to va_start.
-__sentinel bool contains_internal(const wcstring &needle, int vararg_handle, ...) {
-    const wchar_t *arg;
-    va_list va;
-    int res = 0;
-
-    const wchar_t *needle_cstr = needle.c_str();
-    va_start(va, vararg_handle);
-    while ((arg = va_arg(va, const wchar_t *)) != 0) {
-        // libc++ has an unfortunate implementation of operator== that unconditonally wcslen's the
-        // wchar_t* parameter, so prefer wcscmp directly.
-        if (!wcscmp(needle_cstr, arg)) {
-            res = 1;
-            break;
-        }
-    }
-    va_end(va);
-    return res;
-}
-
 long read_blocked(int fd, void *buf, size_t count) {
     long bytes_read = 0;
 
@@ -1588,7 +1549,7 @@ int string_fuzzy_match_t::compare(const string_fuzzy_match_t &rhs) const {
     return 0;  // equal
 }
 
-bool list_contains_string(const wcstring_list_t &list, const wcstring &str) {
+bool contains(const wcstring_list_t &list, const wcstring &str) {
     return std::find(list.begin(), list.end(), str) != list.end();
 }
 

src/common.h

@@ -250,8 +250,8 @@ extern bool has_working_tty_timestamps;
 /// See https://developer.gnome.org/glib/stable/glib-I18N.html#N-:CAPS
 #define N_(wstr) wstr
 
-/// Check if the specified string element is a part of the specified string list.
+/// Test if a list of stirngs contains a particular string.
-#define contains(str, ...) contains_internal(str, 0, __VA_ARGS__, NULL)
+bool contains(const wcstring_list_t &list, const wcstring &str);
 
 /// Print a stack trace to stderr.
 void show_stackframe(const wchar_t msg_level, int frame_count = 100, int skip_levels = 0);
@@ -362,9 +362,6 @@ string_fuzzy_match_t string_fuzzy_match_string(const wcstring &string,
                                                const wcstring &match_against,
                                                fuzzy_match_type_t limit_type = fuzzy_match_none);
 
-/// Test if a list contains a string using a linear search.
-bool list_contains_string(const wcstring_list_t &list, const wcstring &str);
-
 // Check if we are running in the test mode, where we should suppress error output
 #define TESTS_PROGRAM_NAME L"(ignore)"
 bool should_suppress_stderr_for_tests();
@@ -674,15 +671,6 @@ void error_reset();
 /// initialization.
 void fish_setlocale();
 
-/// Checks if \c needle is included in the list of strings specified. A warning is printed if needle
-/// is zero.
-///
-/// \param needle the string to search for in the list.
-///
-/// \return zero if needle is not found, of if needle is null, non-zero otherwise.
-__sentinel bool contains_internal(const wchar_t *needle, int vararg_handle, ...);
-__sentinel bool contains_internal(const wcstring &needle, int vararg_handle, ...);
-
 /// Call read while blocking the SIGCHLD signal. Should only be called if you _know_ there is data
 /// available for reading, or the program will hang until there is data.
 long read_blocked(int fd, void *buf, size_t count);

src/env.cpp

@@ -87,15 +87,15 @@ static bool env_initialized = false;
 
 /// List of all locale environment variable names that might trigger (re)initializing the locale
 /// subsystem.
-static const wchar_t *const locale_variable[] = {
+static const wcstring_list_t locale_variables({L"LANG", L"LANGUAGE", L"LC_ALL", L"LC_ADDRESS",
-    L"LANG",     L"LANGUAGE",          L"LC_ALL",         L"LC_ADDRESS",   L"LC_COLLATE",
+                                               L"LC_COLLATE", L"LC_CTYPE", L"LC_IDENTIFICATION",
-    L"LC_CTYPE", L"LC_IDENTIFICATION", L"LC_MEASUREMENT", L"LC_MESSAGES",  L"LC_MONETARY",
+                                               L"LC_MEASUREMENT", L"LC_MESSAGES", L"LC_MONETARY",
-    L"LC_NAME",  L"LC_NUMERIC",        L"LC_PAPER",       L"LC_TELEPHONE", L"LC_TIME",
+                                               L"LC_NAME", L"LC_NUMERIC", L"LC_PAPER",
-    NULL};
+                                               L"LC_TELEPHONE", L"LC_TIME"});
 
 /// List of all curses environment variable names that might trigger (re)initializing the curses
 /// subsystem.
-static const wchar_t *const curses_variable[] = {L"TERM", L"TERMINFO", L"TERMINFO_DIRS", NULL};
+static const wcstring_list_t curses_variables({L"TERM", L"TERMINFO", L"TERMINFO_DIRS"});
 
 /// List of all "path" like variable names that need special handling. This includes automatic
 /// splitting and joining on import/export. As well as replacing empty elements, which implicitly
@@ -165,7 +165,7 @@ struct var_stack_t {
     // Pops the top node if it's not global
     void pop();
 
-    bool var_changed(wchar_t const *const vars[]);
+    bool var_changed(const wcstring_list_t &vars);
 
     // Returns the next scope to search for a given node, respecting the new_scope lag
     // Returns NULL if we're done
@@ -188,9 +188,9 @@ void var_stack_t::push(bool new_scope) {
 }
 
 /// Return true if one of the vars in the passed list was changed in the current var scope.
-bool var_stack_t::var_changed(wchar_t const *const vars[]) {
+bool var_stack_t::var_changed(const wcstring_list_t &vars) {
-    for (auto v = vars; *v; v++) {
+    for (auto v : vars) {
-        if (top->env.find(*v) != top->env.end()) return true;
+        if (top->env.find(v) != top->env.end()) return true;
     }
     return false;
 }
@@ -203,8 +203,8 @@ void var_stack_t::pop() {
         return;
     }
 
-    bool locale_changed = this->var_changed(locale_variable);
+    bool locale_changed = this->var_changed(locale_variables);
-    bool curses_changed = this->var_changed(curses_variable);
+    bool curses_changed = this->var_changed(curses_variables);
 
     if (top->new_scope) {  //!OCLINT(collapsible if statements)
         if (top->exportv || local_scope_exports(top->next.get())) {
@@ -365,8 +365,8 @@ static void fix_colon_delimited_var(const wcstring &var_name) {
 
 /// Check if the specified variable is a locale variable.
 static bool var_is_locale(const wcstring &key) {
-    for (size_t i = 0; locale_variable[i]; i++) {
+    for (auto var_name : locale_variables) {
-        if (key == locale_variable[i]) {
+        if (key == var_name) {
             return true;
         }
     }
@@ -379,9 +379,9 @@ static void init_locale() {
     // invalidate the pointer from the this setlocale() call.
     char *old_msg_locale = strdup(setlocale(LC_MESSAGES, NULL));
 
-    for (const wchar_t *const *var_name = locale_variable; *var_name; var_name++) {
+    for (auto var_name : locale_variables) {
-        const env_var_t val = env_get_string(*var_name, ENV_EXPORT);
+        const env_var_t val = env_get_string(var_name, ENV_EXPORT);
-        const std::string &name = wcs2string(*var_name);
+        const std::string &name = wcs2string(var_name);
         const std::string &value = wcs2string(val);
         debug(2, L"locale var %s='%s'", name.c_str(), value.c_str());
         if (val.empty()) {
@@ -410,8 +410,8 @@ static void init_locale() {
 
 /// Check if the specified variable is a locale variable.
 static bool var_is_curses(const wcstring &key) {
-    for (size_t i = 0; curses_variable[i]; i++) {
+    for (auto var_name : curses_variables) {
-        if (key == curses_variable[i]) {
+        if (key == var_name) {
             return true;
         }
     }
@@ -433,17 +433,19 @@ bool term_supports_setting_title() { return can_set_term_title; }
 /// One situation in which this breaks down is with screen, since screen supports setting the
 /// terminal title if the underlying terminal does so, but will print garbage on terminals that
 /// don't. Since we can't see the underlying terminal below screen there is no way to fix this.
+static const wcstring_list_t title_terms({L"xterm", L"screen", L"tmux", L"nxterm", L"rxvt"});
 static bool does_term_support_setting_title() {
     const env_var_t term_str = env_get_string(L"TERM");
     if (term_str.missing()) return false;
 
     const wchar_t *term = term_str.c_str();
-    bool recognized = contains(term, L"xterm", L"screen", L"tmux", L"nxterm", L"rxvt");
+    bool recognized = contains(title_terms, term_str);
     if (!recognized) recognized = !wcsncmp(term, L"xterm-", wcslen(L"xterm-"));
     if (!recognized) recognized = !wcsncmp(term, L"screen-", wcslen(L"screen-"));
     if (!recognized) recognized = !wcsncmp(term, L"tmux-", wcslen(L"tmux-"));
     if (!recognized) {
-        if (contains(term, L"linux", L"dumb")) return false;
+        if (term_str == L"linux") return false;
+        if (term_str == L"dumb") return false;
 
         char *n = ttyname(STDIN_FILENO);
         if (!n || strstr(n, "tty") || strstr(n, "/vc/")) return false;
@@ -535,9 +537,9 @@ static void init_path_vars() {
 
 /// Initialize the curses subsystem.
 static void init_curses() {
-    for (const wchar_t *const *var_name = curses_variable; *var_name; var_name++) {
+    for (auto var_name : curses_variables) {
-        const env_var_t val = env_get_string(*var_name, ENV_EXPORT);
+        const env_var_t val = env_get_string(var_name, ENV_EXPORT);
-        const std::string &name = wcs2string(*var_name);
+        const std::string &name = wcs2string(var_name);
         const std::string &value = wcs2string(val);
         debug(2, L"curses var %s='%s'", name.c_str(), value.c_str());
         if (val.empty()) {
@@ -577,7 +579,7 @@ static void init_curses() {
 // outgoing back to it. This is deliberately very short - we don't want to add language-specific
 // values like CLASSPATH.
 static bool variable_is_colon_delimited_var(const wcstring &str) {
-    return list_contains_string(colon_delimited_variable, str);
+    return contains(colon_delimited_variable, str);
 }
 
 /// React to modifying the given variable.
@@ -902,7 +904,7 @@ int env_set(const wcstring &key, const wchar_t *val, env_mode_flags_t var_mode)
     bool has_changed_old = vars_stack().has_changed_exported;
     int done = 0;
 
-    if (val && contains(key, L"PWD", L"HOME")) {
+    if (val && (key == L"PWD" || key == L"HOME")) {
         // Canonicalize our path; if it changes, recurse and try again.
         wcstring val_canonical = val;
         path_make_canonical(val_canonical);

src/history.cpp

@@ -1830,7 +1830,7 @@ void history_t::add_pending_with_file_detection(const wcstring &str) {
             wcstring command;
             tree.command_for_plain_statement(node, str, &command);
             unescape_string_in_place(&command, UNESCAPE_DEFAULT);
-            if (contains(command, L"exit", L"reboot")) {
+            if (command == L"exit" || command == L"reboot") {
                 impending_exit = true;
             }
         }

src/parse_execution.cpp

@@ -875,7 +875,7 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process(
         // If we have defined a wrapper around cd, use it, otherwise use the cd builtin.
         process_type = function_exists(L"cd") ? INTERNAL_FUNCTION : INTERNAL_BUILTIN;
     } else {
-        const globspec_t glob_behavior = contains(cmd, L"set", L"count") ? nullglob : failglob;
+        const globspec_t glob_behavior = (cmd == L"set" || cmd == L"count") ? nullglob : failglob;
         // Form the list of arguments. The command is the first argument. TODO: count hack, where we
         // treat 'count --help' as different from 'count $foo' that expands to 'count --help'. fish
         // 1.x never successfully did this, but it tried to!

src/parse_tree.cpp

@@ -1055,7 +1055,9 @@ static const parse_token_t kInvalidToken = {
 static const parse_token_t kTerminalToken = {
     parse_token_type_terminate, parse_keyword_none, false, false, SOURCE_OFFSET_INVALID, 0};
 
-static inline bool is_help_argument(const wcstring &txt) { return contains(txt, L"-h", L"--help"); }
+static inline bool is_help_argument(const wcstring &txt) {
+    return txt == L"-h" || txt == L"--help";
+}
 
 /// Return a new parse token, advancing the tokenizer.
 static inline parse_token_t next_parse_token(tokenizer_t *tok, tok_t *token) {

src/parse_util.cpp

@@ -748,8 +748,10 @@ static bool append_syntax_error(parse_error_list_t *errors, size_t source_locati
 }
 
 /// Returns 1 if the specified command is a builtin that may not be used in a pipeline.
+static const wcstring_list_t forbidden_pipe_commands({L"exec", L"case", L"break", L"return",
+                                                      L"continue"});
 static int parser_is_pipe_forbidden(const wcstring &word) {
-    return contains(word, L"exec", L"case", L"break", L"return", L"continue");
+    return contains(forbidden_pipe_commands, word);
 }
 
 bool parse_util_argument_is_help(const wchar_t *s, int min_match) {

src/parser_keywords.cpp

@@ -1,24 +1,29 @@
 // Functions having to do with parser keywords, like testing if a function is a block command.
 #include "config.h"  // IWYU pragma: keep
 
+#include <string>
+
 #include "common.h"
 #include "fallback.h"  // IWYU pragma: keep
 #include "parser_keywords.h"
 
 bool parser_keywords_skip_arguments(const wcstring &cmd) {
-    return contains(cmd, L"else", L"begin");
+    return cmd == L"else" || cmd == L"begin";
 }
 
+static const wcstring_list_t subcommand_keywords({L"command", L"builtin", L"while", L"exec", L"if",
+                                                  L"and", L"or", L"not"});
 bool parser_keywords_is_subcommand(const wcstring &cmd) {
-    return parser_keywords_skip_arguments(cmd) ||
+    return parser_keywords_skip_arguments(cmd) || contains(subcommand_keywords, cmd);
-           contains(cmd, L"command", L"builtin", L"while", L"exec", L"if", L"and", L"or", L"not");
 }
 
-bool parser_keywords_is_block(const wcstring &word) {
+static const wcstring_list_t block_keywords({L"for", L"while", L"if", L"function", L"switch",
-    return contains(word, L"for", L"while", L"if", L"function", L"switch", L"begin");
+                                             L"begin"});
-}
+bool parser_keywords_is_block(const wcstring &word) { return contains(block_keywords, word); }
 
+static const wcstring_list_t reserved_keywords({L"end", L"case", L"else", L"return", L"continue",
+                                                L"break"});
 bool parser_keywords_is_reserved(const wcstring &word) {
     return parser_keywords_is_block(word) || parser_keywords_is_subcommand(word) ||
-           contains(word, L"end", L"case", L"else", L"return", L"continue", L"break");
+           contains(reserved_keywords, word);
 }

src/path.cpp

@@ -51,7 +51,10 @@ static bool path_get_path_core(const wcstring &cmd, wcstring *out_path,
     if (!bin_path_var.missing()) {
         bin_path = bin_path_var;
     } else {
-        if (contains(PREFIX L"/bin", L"/bin", L"/usr/bin")) {
+        // Note that PREFIX is defined in the Makefile and is defined when this module is compiled.
+        // This ensures we always default to "/bin", "/usr/bin" and the bin dir defined for the fish
+        // programs with no duplicates.
+        if (!wcscmp(PREFIX L"/bin", L"/bin") || !wcscmp(PREFIX L"/bin", L"/usr/bin")) {
             bin_path = L"/bin" ARRAY_SEP_STR L"/usr/bin";
         } else {
             bin_path = L"/bin" ARRAY_SEP_STR L"/usr/bin" ARRAY_SEP_STR PREFIX L"/bin";

src/wildcard.cpp

@@ -101,7 +101,7 @@ static enum fuzzy_match_type_t wildcard_match_internal(const wchar_t *str, const
 
     // Hackish fix for issue #270. Prevent wildcards from matching . or .., but we must still allow
     // literal matches.
-    if (leading_dots_fail_to_match && is_first && contains(str, L".", L"..")) {
+    if (leading_dots_fail_to_match && is_first && (!wcscmp(str, L".") || !wcscmp(str, L".."))) {
         // The string is '.' or '..'. Return true if the wildcard exactly matches.
         return wcscmp(str, wc) ? fuzzy_match_none : fuzzy_match_exact;
     }
@@ -700,7 +700,7 @@ void wildcard_expander_t::expand_literal_intermediate_segment_with_fuzz(const wc
 
     while (!interrupted() && wreaddir_for_dirs(base_dir_fp, &name_str)) {
         // Don't bother with . and ..
-        if (contains(name_str, L".", L"..")) {
+        if (name_str == L"." || name_str == L"..") {
             continue;
         }