From 082f074bb1a5bce83ad2fed294c41becb1974755 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 13 Aug 2022 12:15:23 -0700 Subject: [PATCH] Switch filenames from intern'd strings to shared_ptr We store filenames in function definitions to indicate where the function comes from. Previously these were intern'd strings. Switch them to a shared_ptr, intending to remove intern'd strings. --- src/builtins/functions.cpp | 14 +++++++------- src/builtins/source.cpp | 19 +++++++++---------- src/builtins/status.cpp | 2 +- src/builtins/type.cpp | 8 ++++---- src/common.h | 3 +++ src/exec.cpp | 4 ++-- src/fish.cpp | 8 +++----- src/function.h | 5 ++--- src/parser.cpp | 31 ++++++++++++++++--------------- src/parser.h | 13 ++++++------- 10 files changed, 53 insertions(+), 54 deletions(-) diff --git a/src/builtins/functions.cpp b/src/builtins/functions.cpp index 16713dffc..49d69f91b 100644 --- a/src/builtins/functions.cpp +++ b/src/builtins/functions.cpp @@ -137,15 +137,15 @@ static int parse_cmd_opts(functions_cmd_opts_t &opts, int *optind, //!OCLINT(hi static int report_function_metadata(const wcstring &funcname, bool verbose, io_streams_t &streams, parser_t &parser, bool metadata_as_comments) { - const wchar_t *path = L"n/a"; + wcstring path = L"n/a"; const wchar_t *autoloaded = L"n/a"; const wchar_t *shadows_scope = L"n/a"; wcstring description = L"n/a"; int line_number = 0; if (auto props = function_get_props_autoload(funcname, parser)) { - path = props->definition_file; - if (path) { + if (props->definition_file) { + path = *props->definition_file; autoloaded = props->is_autoload ? L"autoloaded" : L"not-autoloaded"; line_number = props->definition_lineno(); } else { @@ -159,12 +159,12 @@ static int report_function_metadata(const wcstring &funcname, bool verbose, io_s // "stdin" means it was defined interactively, "-" means it was defined via `source`. // Neither is useful information. wcstring comment; - if (!std::wcscmp(path, L"stdin")) { + if (path == L"stdin") { append_format(comment, L"# Defined interactively\n"); - } else if (!std::wcscmp(path, L"-")) { + } else if (path == L"-") { append_format(comment, L"# Defined via `source`\n"); } else { - append_format(comment, L"# Defined in %ls @ line %d\n", path, line_number); + append_format(comment, L"# Defined in %ls @ line %d\n", path.c_str(), line_number); } if (!streams.out_is_redirected && isatty(STDOUT_FILENO)) { @@ -175,7 +175,7 @@ static int report_function_metadata(const wcstring &funcname, bool verbose, io_s streams.out.append(comment); } } else { - streams.out.append_format(L"%ls\n", path); + streams.out.append_format(L"%ls\n", path.c_str()); if (verbose) { streams.out.append_format(L"%ls\n", autoloaded); streams.out.append_format(L"%d\n", line_number); diff --git a/src/builtins/source.cpp b/src/builtins/source.cpp index deabdb473..2cd81fd5f 100644 --- a/src/builtins/source.cpp +++ b/src/builtins/source.cpp @@ -43,7 +43,7 @@ maybe_t builtin_source(parser_t &parser, io_streams_t &streams, const wchar int fd = -1; struct stat buf; - const wchar_t *fn, *fn_intern; + filename_ref_t func_filename{}; if (argc == optind || std::wcscmp(argv[optind], L"-") == 0) { if (streams.stdin_fd < 0) { @@ -55,8 +55,7 @@ maybe_t builtin_source(parser_t &parser, io_streams_t &streams, const wchar // Don't implicitly read from the terminal. return STATUS_CMD_ERROR; } - fn = L"-"; - fn_intern = fn; + func_filename = std::make_shared(L"-"); fd = streams.stdin_fd; } else { opened_fd = autoclose_fd_t(wopen_cloexec(argv[optind], O_RDONLY)); @@ -83,13 +82,14 @@ maybe_t builtin_source(parser_t &parser, io_streams_t &streams, const wchar return STATUS_CMD_ERROR; } - fn_intern = intern(argv[optind]); + func_filename = std::make_shared(argv[optind]); } assert(fd >= 0 && "Should have a valid fd"); + assert(func_filename && "Should have valid function filename"); - const block_t *sb = parser.push_block(block_t::source_block(fn_intern)); + const block_t *sb = parser.push_block(block_t::source_block(func_filename)); auto &ld = parser.libdata(); - scoped_push filename_push{&ld.current_filename, fn_intern}; + scoped_push filename_push{&ld.current_filename, func_filename}; // Construct argv from our null-terminated list. // This is slightly subtle. If this is a bare `source` with no args then `argv + optind` already @@ -106,10 +106,9 @@ maybe_t builtin_source(parser_t &parser, io_streams_t &streams, const wchar parser.pop_block(sb); if (retval != STATUS_CMD_OK) { - wcstring esc = escape_string(fn_intern); - streams.err.append_format( - _(L"%ls: Error while reading file '%ls'\n"), cmd, - escape_string(fn_intern) == intern_static(L"-") ? L"" : esc.c_str()); + wcstring esc = escape_string(*func_filename); + streams.err.append_format(_(L"%ls: Error while reading file '%ls'\n"), cmd, + esc == L"-" ? L"" : esc.c_str()); } else { retval = parser.get_last_status(); } diff --git a/src/builtins/status.cpp b/src/builtins/status.cpp index 01653136b..640c05541 100644 --- a/src/builtins/status.cpp +++ b/src/builtins/status.cpp @@ -370,7 +370,7 @@ maybe_t builtin_status(parser_t &parser, io_streams_t &streams, const wchar case STATUS_FILENAME: { CHECK_FOR_UNEXPECTED_STATUS_ARGS(opts.status_cmd) auto res = parser.current_filename(); - wcstring fn = res ? res : L""; + wcstring fn = res ? *res : L""; if (!fn.empty() && opts.status_cmd == STATUS_DIRNAME) { fn = wdirname(fn); } else if (!fn.empty() && opts.status_cmd == STATUS_BASENAME) { diff --git a/src/builtins/type.cpp b/src/builtins/type.cpp index a833fd4b9..a8a89fb9f 100644 --- a/src/builtins/type.cpp +++ b/src/builtins/type.cpp @@ -134,7 +134,7 @@ maybe_t builtin_type(parser_t &parser, io_streams_t &streams, const wchar_t auto path = func->definition_file; if (opts.path) { if (path) { - streams.out.append(path); + streams.out.append(*path); streams.out.append(L"\n"); } } else if (!opts.short_output) { @@ -146,8 +146,8 @@ maybe_t builtin_type(parser_t &parser, io_streams_t &streams, const wchar_t if (path) { int line_number = func->definition_lineno(); wcstring comment; - if (std::wcscmp(path, L"-") != 0) { - append_format(comment, L"# Defined in %ls @ line %d\n", path, + if (*path != L"-") { + append_format(comment, L"# Defined in %ls @ line %d\n", path->c_str(), line_number); } else { append_format(comment, L"# Defined via `source`\n"); @@ -169,7 +169,7 @@ maybe_t builtin_type(parser_t &parser, io_streams_t &streams, const wchar_t streams.out.append_format(_(L"%ls is a function"), name); auto path = func->definition_file; if (path) { - streams.out.append_format(_(L" (defined in %ls)"), path); + streams.out.append_format(_(L" (defined in %ls)"), path->c_str()); } streams.out.append(L"\n"); } diff --git a/src/common.h b/src/common.h index 46ebac2ae..feec54c6b 100644 --- a/src/common.h +++ b/src/common.h @@ -337,6 +337,9 @@ 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); +/// Stored in blocks to reference the file which created the block. +using filename_ref_t = std::shared_ptr; + using scoped_lock = std::lock_guard; // An object wrapping a scoped lock and a value diff --git a/src/exec.cpp b/src/exec.cpp index 735eb69d3..b4c1b90bb 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -522,7 +522,7 @@ static launch_result_t exec_external_command(parser_t &parser, const std::shared std::string actual_cmd_str = wcs2string(p->actual_cmd); const char *actual_cmd = actual_cmd_str.c_str(); - const wchar_t *file = parser.libdata().current_filename; + filename_ref_t file = parser.libdata().current_filename; #if FISH_USE_POSIX_SPAWN // Prefer to use posix_spawn, since it's faster on some systems like OS X. @@ -544,7 +544,7 @@ static launch_result_t exec_external_command(parser_t &parser, const std::shared // usleep(10000); FLOGF(exec_fork, L"Fork #%d, pid %d: spawn external command '%s' from '%ls'", - int(s_fork_count), *pid, actual_cmd, file ? file : L""); + int(s_fork_count), *pid, actual_cmd, file ? file->c_str() : L""); // these are all things do_fork() takes care of normally (for forked processes): p->pid = *pid; diff --git a/src/fish.cpp b/src/fish.cpp index 0255fe6b5..5c0cef642 100644 --- a/src/fish.cpp +++ b/src/fish.cpp @@ -585,13 +585,11 @@ int main(int argc, char **argv) { parser.vars().set(L"argv", ENV_DEFAULT, std::move(list)); auto &ld = parser.libdata(); - wcstring rel_filename = str2wcstring(file); - scoped_push filename_push{&ld.current_filename, - intern(rel_filename.c_str())}; + filename_ref_t rel_filename = std::make_shared(str2wcstring(file)); + scoped_push filename_push{&ld.current_filename, rel_filename}; res = reader_read(parser, fd.fd(), {}); if (res) { - FLOGF(warning, _(L"Error while reading file %ls\n"), - ld.current_filename ? ld.current_filename : _(L"Standard input")); + FLOGF(warning, _(L"Error while reading file %ls\n"), rel_filename->c_str()); } } } diff --git a/src/function.h b/src/function.h index ba422df84..1770c0567 100644 --- a/src/function.h +++ b/src/function.h @@ -44,9 +44,8 @@ struct function_properties_t { /// Whether the function was autoloaded. bool is_autoload{false}; - /// The file from which the function was created (intern'd string), or nullptr if not from a - /// file. - const wchar_t *definition_file{}; + /// The file from which the function was created, or nullptr if not from a file. + filename_ref_t definition_file{}; /// \return the description, localized via _. const wchar_t *localized_description() const; diff --git a/src/parser.cpp b/src/parser.cpp index bafaaf9b4..e9843ff69 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -241,9 +241,9 @@ static void append_block_description_to_stack_trace(const parser_t &parser, cons break; } case block_type_t::source: { - const wchar_t *source_dest = b.sourced_file; + const filename_ref_t &source_dest = b.sourced_file; append_format(trace, _(L"from sourcing file %ls\n"), - user_presentable_path(source_dest, parser.vars()).c_str()); + user_presentable_path(*source_dest, parser.vars()).c_str()); print_call_site = true; break; } @@ -268,10 +268,10 @@ static void append_block_description_to_stack_trace(const parser_t &parser, cons if (print_call_site) { // Print where the function is called. - const wchar_t *file = b.src_filename; + const auto &file = b.src_filename; if (file) { append_format(trace, _(L"\tcalled on line %d of file %ls\n"), b.src_lineno, - user_presentable_path(file, parser.vars()).c_str()); + user_presentable_path(*file, parser.vars()).c_str()); } else if (parser.libdata().within_fish_init) { append_format(trace, _(L"\tcalled during startup\n")); } @@ -377,7 +377,7 @@ int parser_t::get_lineno() const { return lineno; } -const wchar_t *parser_t::current_filename() const { +filename_ref_t parser_t::current_filename() const { for (const auto &b : block_list) { if (b.is_function_call()) { auto props = function_get_props(b.function_name); @@ -415,7 +415,7 @@ wcstring parser_t::current_line() { } const int lineno = this->get_lineno(); - const wchar_t *file = this->current_filename(); + filename_ref_t file = this->current_filename(); wcstring prefix; @@ -423,7 +423,7 @@ wcstring parser_t::current_line() { if (!is_interactive() || is_function()) { if (file) { append_format(prefix, _(L"%ls (line %d): "), - user_presentable_path(file, vars()).c_str(), lineno); + user_presentable_path(*file, vars()).c_str(), lineno); } else if (libdata().within_fish_init) { append_format(prefix, L"%ls (line %d): ", _(L"Startup"), lineno); } else { @@ -637,14 +637,15 @@ void parser_t::get_backtrace(const wcstring &src, const parse_error_list_t &erro } wcstring prefix; - const wchar_t *filename = this->current_filename(); + filename_ref_t filename = this->current_filename(); if (filename) { if (which_line > 0) { - prefix = format_string(_(L"%ls (line %lu): "), - user_presentable_path(filename, vars()).c_str(), which_line); + prefix = + format_string(_(L"%ls (line %lu): "), + user_presentable_path(*filename, vars()).c_str(), which_line); } else { prefix = - format_string(_(L"%ls: "), user_presentable_path(filename, vars()).c_str()); + format_string(_(L"%ls: "), user_presentable_path(*filename, vars()).c_str()); } } else { prefix = L"fish: "; @@ -724,8 +725,8 @@ wcstring block_t::description() const { if (this->src_lineno >= 0) { append_format(result, L" (line %d)", this->src_lineno); } - if (this->src_filename != nullptr) { - append_format(result, L" (file %ls)", this->src_filename); + if (this->src_filename) { + append_format(result, L" (file %ls)", this->src_filename->c_str()); } return result; } @@ -747,9 +748,9 @@ block_t block_t::function_block(wcstring name, wcstring_list_t args, bool shadow return b; } -block_t block_t::source_block(const wchar_t *src) { +block_t block_t::source_block(filename_ref_t src) { block_t b{block_type_t::source}; - b.sourced_file = src; + b.sourced_file = std::move(src); return b; } diff --git a/src/parser.h b/src/parser.h index 39e674f9b..f75be1b83 100644 --- a/src/parser.h +++ b/src/parser.h @@ -66,8 +66,8 @@ class block_t { const block_type_t block_type; public: - /// Name of file that created this block. This string is intern'd. - const wchar_t *src_filename{nullptr}; + /// Name of file that created this block. + filename_ref_t src_filename{}; /// Line number where this block was created. int src_lineno{0}; /// Whether we should pop the environment variable stack when we're popped off of the block @@ -83,7 +83,7 @@ class block_t { // If this is a source block, the source'd file, interned. // Otherwise nothing. - const wchar_t *sourced_file{}; + filename_ref_t sourced_file{}; // If this is an event block, the event. Otherwise ignored. maybe_t event; @@ -103,7 +103,7 @@ class block_t { static block_t if_block(); static block_t event_block(event_t evt); static block_t function_block(wcstring name, wcstring_list_t args, bool shadows); - static block_t source_block(const wchar_t *src); + static block_t source_block(filename_ref_t src); static block_t for_block(); static block_t while_block(); static block_t switch_block(); @@ -201,8 +201,7 @@ struct library_data_t { size_t read_limit{0}; /// The current filename we are evaluating, either from builtin source or on the command line. - /// This is an intern'd string. - const wchar_t *current_filename{}; + filename_ref_t current_filename{}; /// List of events that have been sent but have not yet been delivered because they are blocked. std::vector> blocked_events{}; @@ -431,7 +430,7 @@ class parser_t : public std::enable_shared_from_this { /// Returns the file currently evaluated by the parser. This can be different than /// reader_current_filename, e.g. if we are evaluating a function defined in a different file /// than the one curently read. - const wchar_t *current_filename() const; + filename_ref_t current_filename() const; /// Return if we are interactive, which means we are executing a command that the user typed in /// (and not, say, a prompt).