lint: misc cleanups

Earlier lint cleanups overlooked a couple of modules because on macOS at
the moment oclint ignores them. I noticed this when I ran `make lint-all`
on Ubuntu.

osx/osx_fish_launcher.m

@@ -87,7 +87,8 @@ int main(void) {
 
     @autoreleasepool {
         /* Get the fish executable. Make sure it's absolute. */
-        NSURL *fish_executable = [[NSBundle mainBundle] URLForResource:@"fish" withExtension:@"" subdirectory:@"base/bin"];
+        NSURL *fish_executable = [[NSBundle mainBundle] URLForResource:@"fish" withExtension:@""
+                                                          subdirectory:@"base/bin"];
         if (! fish_executable)
             die("Could not find fish executable in bundle");
 

src/builtin.cpp

@@ -3132,7 +3132,8 @@ int builtin_parse(parser_t &parser, io_streams_t &streams, wchar_t **argv)
         const wcstring src = str2wcstring(&txt.at(0), txt.size());
         parse_node_tree_t parse_tree;
         parse_error_list_t errors;
-        bool success = parse_tree_from_string(src, parse_flag_include_comments, &parse_tree, &errors);
+        bool success = parse_tree_from_string(src, parse_flag_include_comments, &parse_tree,
+                                              &errors);
         if (! success)
         {
             streams.out.append(L"Parsing failed:\n");
@@ -3145,7 +3146,8 @@ int builtin_parse(parser_t &parser, io_streams_t &streams, wchar_t **argv)
             streams.out.append(L"(Reparsed with continue after error)\n");
             parse_tree.clear();
             errors.clear();
-            parse_tree_from_string(src, parse_flag_continue_after_error | parse_flag_include_comments, &parse_tree, &errors);
+            parse_tree_from_string(src, parse_flag_continue_after_error |
+                                   parse_flag_include_comments, &parse_tree, &errors);
         }
         const wcstring dump = parse_dump_tree(parse_tree, src);
         streams.out.append(dump);

src/env.cpp

@@ -581,11 +581,10 @@ int env_set(const wcstring &key, const wchar_t *val, env_mode_flags_t var_mode)
             node = top;
         } else if (preexisting_node != NULL) {
             node = preexisting_node;
-
             if ((var_mode & (ENV_EXPORT | ENV_UNEXPORT)) == 0) {
                 // use existing entry's exportv
-                var_mode =
+                var_mode =  //!OCLINT(parameter reassignment)
-                    preexisting_entry_exportv ? ENV_EXPORT : 0;  //!OCLINT(parameter reassignment)
+                    preexisting_entry_exportv ? ENV_EXPORT : 0;
             }
         } else {
             if (!get_proc_had_barrier()) {

src/fallback.cpp

@@ -203,13 +203,10 @@ size_t wcslcpy(wchar_t *dst, const wchar_t *src, size_t siz) {
 
     // Not enough room in dst, add NUL and traverse rest of src.
     if (n == 0) {
-        if (siz != 0) *d = '\0';
+        if (siz != 0) *d = '\0';  // NUL-terminate dst
-        // NUL-terminate dst.
+        while (*s++) ;  // ignore rest of src
-        while (*s++)
-            ;
     }
-    return s - src - 1;
+    return s - src - 1;  // count does not include NUL
-    // Count does not include NUL.
 }
 #endif
 

src/fish_key_reader.cpp

@@ -206,9 +206,8 @@ static void process_input(bool continuous_mode) {
             output_bind_command(bind_chars);
             if (first_char_seen && !continuous_mode) {
                 return;
-            } else {
-                continue;
             }
+            continue;
         }
 
         prev_tstamp = output_elapsed_time(prev_tstamp, first_char_seen);
@@ -307,11 +306,13 @@ int main(int argc, char **argv) {
                                        {"help", no_argument, NULL, 'h'},
                                        {NULL, 0, NULL, 0}};
     int opt;
-    while ((opt = getopt_long(argc, argv, short_opts, long_opts, NULL)) != -1) {
+    bool error = false;
+    while (!error && (opt = getopt_long(argc, argv, short_opts, long_opts, NULL)) != -1) {
         switch (opt) {
             case 0: {
                 fprintf(stderr, "getopt_long() unexpectedly returned zero\n");
-                exit(1);
+                error = true;
+                break;
             }
             case 'c': {
                 continuous_mode = true;
@@ -320,6 +321,7 @@ int main(int argc, char **argv) {
             case 'h': {
                 print_help("fish_key_reader", 0);
                 exit(0);
+                break;
             }
             case 'd': {
                 char *end;
@@ -332,7 +334,7 @@ int main(int argc, char **argv) {
                     debug_level = (int)tmp;
                 } else {
                     fwprintf(stderr, _(L"Invalid value '%s' for debug-level flag"), optarg);
-                    exit(1);
+                    error = true;
                 }
                 break;
             }
@@ -347,16 +349,19 @@ int main(int argc, char **argv) {
                     debug_stack_frames = (int)tmp;
                 } else {
                     fwprintf(stderr, _(L"Invalid value '%s' for debug-stack-frames flag"), optarg);
-                    exit(1);
+                    error = true;
+                    break;
                 }
                 break;
             }
             default: {
                 // We assume getopt_long() has already emitted a diagnostic msg.
-                exit(1);
+                error = true;
+                break;
             }
         }
     }
+    if (error) return 1;
 
     argc -= optind;
     if (argc != 0) {

src/fish_tests.cpp

@@ -154,19 +154,22 @@ static int chdir_set_pwd(const char *path) {
     return ret;
 }
 
+// The odd formulation of these macros is to avoid "multiple unary operator" warnings from oclint
+// were we to use the more natural "if (!(e)) err(..." form. We have to do this because the rules
+// for the C preprocessor make it practically impossible to embed a comment in the body of a macro.
 #define do_test(e)                                                   \
     do {                                                             \
-        if (!(e)) err(L"Test failed on line %lu: %s", __LINE__, #e); \
+        if (e) { ; } else { err(L"Test failed on line %lu: %s", __LINE__, #e); } \
     } while (0)
 
-#define do_test_from(e, from_line)                                                         \
+#define do_test_from(e, from)                                                         \
     do {                                                                                   \
-        if (!(e)) err(L"Test failed on line %lu (from %lu): %s", __LINE__, from_line, #e); \
+        if (e) { ; } else { err(L"Test failed on line %lu (from %lu): %s", __LINE__, from, #e); } \
     } while (0)
 
 #define do_test1(e, msg)                                                 \
     do {                                                                 \
-        if (!(e)) err(L"Test failed on line %lu: %ls", __LINE__, (msg)); \
+        if (e) { ; } else { err(L"Test failed on line %lu: %ls", __LINE__, (msg)); } \
     } while (0)
 
 /// Test sane escapes.
@@ -199,7 +202,7 @@ static void test_unescape_sane() {
         err(L"Should not have been able to unescape \\U110000\n");
     }
     if (is_wchar_ucs2()) {
-        // TODO: Make this work on MS Windows.
+        ; // TODO: Make this work on MS Windows.
     } else {
         if (!unescape_string(L"echo \\U10FFFF", &output, UNESCAPE_DEFAULT)) {
             err(L"Should have been able to unescape \\U10FFFF\n");
@@ -484,7 +487,7 @@ static parser_test_error_bits_t detect_argument_errors(const wcstring &src) {
         return PARSER_TEST_ERROR;
     }
 
-    assert(!tree.empty());
+    assert(!tree.empty());  //!OCLINT(multiple unary operator)
     const parse_node_t *first_arg = tree.next_node_in_node_list(tree.at(0), symbol_argument, NULL);
     assert(first_arg != NULL);
     return parse_util_detect_errors_in_argument(*first_arg, first_arg->get_source(src));
@@ -493,7 +496,6 @@ static parser_test_error_bits_t detect_argument_errors(const wcstring &src) {
 /// Test the parser.
 static void test_parser() {
     say(L"Testing parser");
-    parser_t parser;
 
     say(L"Testing block nesting");
     if (!parse_util_detect_errors(L"if; end")) {
@@ -627,7 +629,8 @@ static void test_parser() {
 #if 0
     // This is disabled since it produces a long backtrace. We should find a way to either visually
     // compress the backtrace, or disable error spewing.
-    parser_t::principal_parser().eval(L"function recursive1 ; recursive2 ; end ; function recursive2 ; recursive1 ; end ; recursive1; ", io_chain_t(), TOP);
+    parser_t::principal_parser().eval(L"function recursive1 ; recursive2 ; end ; "
+            L"function recursive2 ; recursive1 ; end ; recursive1; ", io_chain_t(), TOP);
 #endif
 
     say(L"Testing empty function name");
@@ -840,8 +843,8 @@ static void test_utf82wchar(const char *src, size_t slen, const wchar_t *dst, si
         const unsigned char astral_mask = 0xF0;
         for (size_t i = 0; i < slen; i++) {
             if ((src[i] & astral_mask) == astral_mask) {
-                // Astral char. We expect this conversion to just fail.
+                // Astral char. We want this conversion to fail.
-                res = 0;
+                res = 0;  //!OCLINT(parameter reassignment)
                 break;
             }
         }
@@ -888,8 +891,8 @@ static void test_wchar2utf8(const wchar_t *src, size_t slen, const char *dst, si
         const uint32_t astral_mask = 0xFFFF0000U;
         for (size_t i = 0; i < slen; i++) {
             if ((src[i] & astral_mask) != 0) {
-                /* astral char */
+                // Astral char. We want this conversion to fail.
-                res = 0;
+                res = 0;  //!OCLINT(parameter reassignment)
                 break;
             }
         }
@@ -1137,7 +1140,8 @@ static bool expand_test(const wchar_t *in, expand_flags_t flags, ...) {
     }
 
     if (!res) {
-        if ((arg = va_arg(va, wchar_t *)) != 0) {
+        arg = va_arg(va, wchar_t *);
+        if (arg) {
             wcstring msg = L"Expected [";
             bool first = true;
             for (wcstring_list_t::const_iterator it = expected.begin(), end = expected.end();
@@ -2207,7 +2211,6 @@ static void test_history_matches(history_search_t &search, size_t matches, unsig
     size_t i;
     for (i = 0; i < matches; i++) {
         do_test(search.go_backwards());
-        wcstring item = search.current_string();
     }
     // do_test_from(!search.go_backwards(), from_line);
     bool result = search.go_backwards();
@@ -2403,7 +2406,7 @@ static void trigger_or_wait_for_notification(universal_notifier_t *notifier,
                                              universal_notifier_t::notifier_strategy_t strategy) {
     switch (strategy) {
         case universal_notifier_t::strategy_default: {
-            assert(0 && "strategy_default should be passed");
+            DIE("strategy_default should be passed");
             break;
         }
         case universal_notifier_t::strategy_shmem_polling: {
@@ -3061,35 +3064,36 @@ static bool test_1_parse_ll2(const wcstring &src, wcstring *out_cmd, wcstring *o
     out_joined_args->clear();
     *out_deco = parse_statement_decoration_none;
 
-    bool result = false;
     parse_node_tree_t tree;
-    if (parse_tree_from_string(src, parse_flag_none, &tree, NULL)) {
+    if (!parse_tree_from_string(src, parse_flag_none, &tree, NULL)) {
-        // Get the statement. Should only have one.
+        return false;
-        const parse_node_tree_t::parse_node_list_t stmt_nodes =
-            tree.find_nodes(tree.at(0), symbol_plain_statement);
-        if (stmt_nodes.size() != 1) {
-            say(L"Unexpected number of statements (%lu) found in '%ls'", stmt_nodes.size(),
-                src.c_str());
-            return false;
-        }
-        const parse_node_t &stmt = *stmt_nodes.at(0);
-
-        // Return its decoration.
-        *out_deco = tree.decoration_for_plain_statement(stmt);
-
-        // Return its command.
-        tree.command_for_plain_statement(stmt, src, out_cmd);
-
-        // Return arguments separated by spaces.
-        const parse_node_tree_t::parse_node_list_t arg_nodes =
-            tree.find_nodes(stmt, symbol_argument);
-        for (size_t i = 0; i < arg_nodes.size(); i++) {
-            if (i > 0) out_joined_args->push_back(L' ');
-            out_joined_args->append(arg_nodes.at(i)->get_source(src));
-        }
-        result = true;
     }
-    return result;
+
+    // Get the statement. Should only have one.
+    const parse_node_tree_t::parse_node_list_t stmt_nodes =
+        tree.find_nodes(tree.at(0), symbol_plain_statement);
+    if (stmt_nodes.size() != 1) {
+        say(L"Unexpected number of statements (%lu) found in '%ls'", stmt_nodes.size(),
+            src.c_str());
+        return false;
+    }
+    const parse_node_t &stmt = *stmt_nodes.at(0);
+
+    // Return its decoration.
+    *out_deco = tree.decoration_for_plain_statement(stmt);
+
+    // Return its command.
+    tree.command_for_plain_statement(stmt, src, out_cmd);
+
+    // Return arguments separated by spaces.
+    const parse_node_tree_t::parse_node_list_t arg_nodes =
+        tree.find_nodes(stmt, symbol_argument);
+    for (size_t i = 0; i < arg_nodes.size(); i++) {
+        if (i > 0) out_joined_args->push_back(L' ');
+        out_joined_args->append(arg_nodes.at(i)->get_source(src));
+    }
+
+    return true;
 }
 
 // Test the LL2 (two token lookahead) nature of the parser by exercising the special builtin and
@@ -3253,29 +3257,31 @@ static wcstring_list_t separate_by_format_specifiers(const wchar_t *format) {
 
         // Walk over the format specifier (if any).
         cursor = next_specifier;
-        if (*cursor == '%') {
+        if (*cursor != '%') {
-            cursor++;
+            continue;
-            // Flag
-            if (wcschr(L"#0- +'", *cursor)) cursor++;
-            // Minimum field width
-            while (iswdigit(*cursor)) cursor++;
-            // Precision
-            if (*cursor == L'.') {
-                cursor++;
-                while (iswdigit(*cursor)) cursor++;
-            }
-            // Length modifier
-            if (!wcsncmp(cursor, L"ll", 2) || !wcsncmp(cursor, L"hh", 2)) {
-                cursor += 2;
-            } else if (wcschr(L"hljtzqL", *cursor)) {
-                cursor++;
-            }
-            // The format specifier itself. We allow any character except NUL.
-            if (*cursor != L'\0') {
-                cursor += 1;
-            }
-            assert(cursor <= end);
         }
+
+        cursor++;
+        // Flag
+        if (wcschr(L"#0- +'", *cursor)) cursor++;
+        // Minimum field width
+        while (iswdigit(*cursor)) cursor++;
+        // Precision
+        if (*cursor == L'.') {
+            cursor++;
+            while (iswdigit(*cursor)) cursor++;
+        }
+        // Length modifier
+        if (!wcsncmp(cursor, L"ll", 2) || !wcsncmp(cursor, L"hh", 2)) {
+            cursor += 2;
+        } else if (wcschr(L"hljtzqL", *cursor)) {
+            cursor++;
+        }
+        // The format specifier itself. We allow any character except NUL.
+        if (*cursor != L'\0') {
+            cursor += 1;
+        }
+        assert(cursor <= end);
     }
     return result;
 }

src/highlight.cpp

@@ -52,11 +52,15 @@ static const wchar_t *const highlight_var[] = {
 
 };
 
-/// Determine if the filesystem containing the given fd is case insensitive.
+/// Determine if the filesystem containing the given fd is case insensitive for lookups regardless
+/// of whether it preserves the case when saving a pathname.
+///
+/// Returns:
+///     false: the filesystem is not case insensitive
+///     true: the file system is case insensitive
 typedef std::map<wcstring, bool> case_sensitivity_cache_t;
 bool fs_is_case_insensitive(const wcstring &path, int fd,
                             case_sensitivity_cache_t &case_sensitivity_cache) {
-    // If _PC_CASE_SENSITIVE is not defined, assume case sensitive.
     bool result = false;
 #ifdef _PC_CASE_SENSITIVE
     // Try the cache first.
@@ -71,6 +75,11 @@ bool fs_is_case_insensitive(const wcstring &path, int fd,
         result = (ret == 0);
         case_sensitivity_cache[path] = result;
     }
+#else
+    // Silence lint tools about the unused parameters.
+    UNUSED(path);
+    UNUSED(fd);
+    UNUSED(case_sensitivity_cache);
 #endif
     return result;
 }

src/output.cpp

@@ -440,7 +440,8 @@ rgb_color_t parse_color(const wcstring &val, bool is_background) {
 
 #if 0
     wcstring desc = result.description();
-    printf("Parsed %ls from %ls (%s)\n", desc.c_str(), val.c_str(), is_background ? "background" : "foreground");
+    printf("Parsed %ls from %ls (%s)\n", desc.c_str(), val.c_str(),
+           is_background ? "background" : "foreground");
 #endif
 
     return result;

src/parse_tree.cpp

@@ -1276,7 +1276,8 @@ bool parse_tree_from_string(const wcstring &str, parse_tree_flags_t parse_flags,
 #if 0
     //wcstring result = dump_tree(this->parser->nodes, str);
     //fprintf(stderr, "Tree (%ld nodes):\n%ls", this->parser->nodes.size(), result.c_str());
-    fprintf(stderr, "%lu nodes, node size %lu, %lu bytes\n", output->size(), sizeof(parse_node_t), output->size() * sizeof(parse_node_t));
+    fprintf(stderr, "%lu nodes, node size %lu, %lu bytes\n", output->size(), sizeof(parse_node_t),
+            output->size() * sizeof(parse_node_t));
 #endif
 
     // Indicate if we had a fatal error.