From 1fb8f4e277cf6430a1128095e650526a912daf79 Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Fri, 4 Nov 2016 18:40:22 -0700 Subject: [PATCH] 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 | 19 +++--- src/builtin.cpp | 6 +- src/env.cpp | 5 +- src/fallback.cpp | 9 +-- src/fish_key_reader.cpp | 19 +++--- src/fish_tests.cpp | 134 +++++++++++++++++++++------------------- src/highlight.cpp | 13 +++- src/output.cpp | 3 +- src/parse_tree.cpp | 3 +- 9 files changed, 116 insertions(+), 95 deletions(-) diff --git a/osx/osx_fish_launcher.m b/osx/osx_fish_launcher.m index 20b6e61b0..875ae75f7 100644 --- a/osx/osx_fish_launcher.m +++ b/osx/osx_fish_launcher.m @@ -18,11 +18,11 @@ static void die(const char *format, ...) { vfprintf(stderr, format, ap); va_end(ap); fputc('\n', stderr); - + if (s_command_path[0] != '\0') { unlink(s_command_path); } - + exit(EXIT_FAILURE); } @@ -31,14 +31,14 @@ static void launch_fish_with_applescript(NSString *fish_binary_path) // load the script from a resource by fetching its URL from within our bundle NSString *path = [[NSBundle mainBundle] pathForResource:@"launch_fish" ofType:@"scpt"]; if (! path) die("Couldn't get path to launch_fish.scpt"); - + NSURL *url = [NSURL fileURLWithPath:path isDirectory:NO]; if (! url) die("Couldn't get URL to launch_fish.scpt"); - + NSDictionary *errors = nil; NSAppleScript *appleScript = [[NSAppleScript alloc] initWithContentsOfURL:url error:&errors]; if (! appleScript) die("Couldn't load AppleScript"); - + // create the first parameter NSAppleEventDescriptor *firstParameter = [NSAppleEventDescriptor descriptorWithString:fish_binary_path]; @@ -84,16 +84,17 @@ static void launch_fish_with_applescript(NSString *fish_binary_path) /* This approach asks Terminal to open a script that we control */ 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"); - + launch_fish_with_applescript([fish_executable path]); } - + /* If we succeeded, it will clean itself up */ return 0; } diff --git a/src/builtin.cpp b/src/builtin.cpp index caca9928d..b7d12352c 100644 --- a/src/builtin.cpp +++ b/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); diff --git a/src/env.cpp b/src/env.cpp index 5a9770f92..0fcee01c5 100644 --- a/src/env.cpp +++ b/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 = - preexisting_entry_exportv ? ENV_EXPORT : 0; //!OCLINT(parameter reassignment) + var_mode = //!OCLINT(parameter reassignment) + preexisting_entry_exportv ? ENV_EXPORT : 0; } } else { if (!get_proc_had_barrier()) { diff --git a/src/fallback.cpp b/src/fallback.cpp index a9e9140f3..255d8d95f 100644 --- a/src/fallback.cpp +++ b/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'; - // NUL-terminate dst. - while (*s++) - ; + if (siz != 0) *d = '\0'; // NUL-terminate dst + while (*s++) ; // ignore rest of src } - return s - src - 1; - // Count does not include NUL. + return s - src - 1; // count does not include NUL } #endif diff --git a/src/fish_key_reader.cpp b/src/fish_key_reader.cpp index c2deb5076..281396638 100644 --- a/src/fish_key_reader.cpp +++ b/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) { diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 99a0741ae..f08eeecbf 100644 --- a/src/fish_tests.cpp +++ b/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. - res = 0; + // Astral char. We want this conversion to fail. + 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 */ - res = 0; + // Astral char. We want this conversion to fail. + 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)) { - // 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)); - } - result = true; + if (!parse_tree_from_string(src, parse_flag_none, &tree, NULL)) { + return false; } - 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 == '%') { - 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); + if (*cursor != '%') { + continue; } + + 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; } diff --git a/src/highlight.cpp b/src/highlight.cpp index 6033de4d4..85844dc1c 100644 --- a/src/highlight.cpp +++ b/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 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; } diff --git a/src/output.cpp b/src/output.cpp index 039d32ce5..707a922aa 100644 --- a/src/output.cpp +++ b/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; diff --git a/src/parse_tree.cpp b/src/parse_tree.cpp index cb197ae26..3a0ceb168 100644 --- a/src/parse_tree.cpp +++ b/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.