diff --git a/src/builtin_bg.cpp b/src/builtin_bg.cpp index c97b1dddb..9af0cbbfc 100644 --- a/src/builtin_bg.cpp +++ b/src/builtin_bg.cpp @@ -51,19 +51,19 @@ int builtin_bg(parser_t &parser, io_streams_t &streams, wchar_t **argv) { if (optind == argc) { // No jobs were specified so use the most recent (i.e., last) job. - job_t *j; - job_iterator_t jobs; - while ((j = jobs.next())) { + job_t *job = nullptr; + for (auto j : jobs()) { if (j->is_stopped() && j->get_flag(job_flag_t::JOB_CONTROL) && (!j->is_completed())) { + job = j.get(); break; } } - if (!j) { + if (!job) { streams.err.append_format(_(L"%ls: There are no suitable jobs\n"), cmd); retval = STATUS_CMD_ERROR; } else { - retval = send_to_bg(parser, streams, j); + retval = send_to_bg(parser, streams, job); } return retval; diff --git a/src/builtin_disown.cpp b/src/builtin_disown.cpp index 52e2c995b..dc766cc34 100644 --- a/src/builtin_disown.cpp +++ b/src/builtin_disown.cpp @@ -31,13 +31,17 @@ static int disown_job(const wchar_t *cmd, parser_t &parser, io_streams_t &stream streams.err.append_format(fmt, cmd, j->job_id, j->command_wcstr()); } - pid_t pgid = j->pgid; - if (!parser.job_remove(j)) { - return STATUS_CMD_ERROR; + for (auto itr = jobs().begin(); itr != jobs().end(); ++itr) { + auto job = itr->get(); + if (job == j) { + pid_t pgid = j->pgid; + add_disowned_pgid(pgid); + jobs().erase(itr); + return STATUS_CMD_OK; + } } - add_disowned_pgid(pgid); - return STATUS_CMD_OK; + return STATUS_CMD_ERROR; } /// Builtin for removing jobs from the job list. @@ -56,20 +60,20 @@ int builtin_disown(parser_t &parser, io_streams_t &streams, wchar_t **argv) { } if (argv[1] == 0) { - job_t *j; // Select last constructed job (ie first job in the job queue) that is possible to disown. // Stopped jobs can be disowned (they will be continued). // Foreground jobs can be disowned. // Even jobs that aren't under job control can be disowned! - job_iterator_t jobs; - while ((j = jobs.next())) { + job_t *job = nullptr; + for (auto j : jobs()) { if (j->is_constructed() && (!j->is_completed())) { + job = j.get(); break; } } - if (j) { - retval = disown_job(cmd, parser, streams, j); + if (job) { + retval = disown_job(cmd, parser, streams, job); } else { streams.err.append_format(_(L"%ls: There are no suitable jobs\n"), cmd); retval = STATUS_CMD_ERROR; diff --git a/src/builtin_fg.cpp b/src/builtin_fg.cpp index 8a915ecc1..0ef092740 100644 --- a/src/builtin_fg.cpp +++ b/src/builtin_fg.cpp @@ -33,20 +33,20 @@ int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) { return STATUS_CMD_OK; } - job_t *j = NULL; - + job_t *job = nullptr; if (optind == argc) { // Select last constructed job (I.e. first job in the job que) that is possible to put in // the foreground. - job_iterator_t jobs; - while ((j = jobs.next())) { + + for (auto j : jobs()) { if (j->is_constructed() && (!j->is_completed()) && ((j->is_stopped() || (!j->is_foreground())) && j->get_flag(job_flag_t::JOB_CONTROL))) { + job = j.get(); break; } } - if (!j) { + if (!job) { streams.err.append_format(_(L"%ls: There are no suitable jobs\n"), cmd); } } else if (optind + 1 < argc) { @@ -58,8 +58,8 @@ int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) { pid = fish_wcstoi(argv[optind]); if (!(errno || pid < 0)) { - j = job_t::from_pid(pid); - if (j) found_job = true; + job = job_t::from_pid(pid); + if (job) found_job = true; } if (found_job) { @@ -70,46 +70,46 @@ int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) { builtin_print_error_trailer(parser, streams.err, cmd); - j = 0; + job = 0; } else { int pid = abs(fish_wcstoi(argv[optind])); if (errno) { streams.err.append_format(BUILTIN_ERR_NOT_NUMBER, cmd, argv[optind]); builtin_print_error_trailer(parser, streams.err, cmd); } else { - j = job_t::from_pid(pid); - if (!j || !j->is_constructed() || j->is_completed()) { + job = job_t::from_pid(pid); + if (!job || !job->is_constructed() || job->is_completed()) { streams.err.append_format(_(L"%ls: No suitable job: %d\n"), cmd, pid); - j = 0; - } else if (!j->get_flag(job_flag_t::JOB_CONTROL)) { + job = nullptr; + } else if (!job->get_flag(job_flag_t::JOB_CONTROL)) { streams.err.append_format(_(L"%ls: Can't put job %d, '%ls' to foreground because " L"it is not under job control\n"), - cmd, pid, j->command_wcstr()); - j = 0; + cmd, pid, job->command_wcstr()); + job = 0; } } } - if (!j) { + if (!job) { return STATUS_INVALID_ARGS; } if (streams.err_is_redirected) { - streams.err.append_format(FG_MSG, j->job_id, j->command_wcstr()); + streams.err.append_format(FG_MSG, job->job_id, job->command_wcstr()); } else { // If we aren't redirecting, send output to real stderr, since stuff in sb_err won't get // printed until the command finishes. - std::fwprintf(stderr, FG_MSG, j->job_id, j->command_wcstr()); + std::fwprintf(stderr, FG_MSG, job->job_id, job->command_wcstr()); } - const wcstring ft = tok_first(j->command()); + const wcstring ft = tok_first(job->command()); //For compatibility with fish 2.0's $_, now replaced with `status current-command` if (!ft.empty()) parser.vars().set_one(L"_", ENV_EXPORT, ft); - reader_write_title(j->command()); + reader_write_title(job->command()); - j->promote(); - j->set_flag(job_flag_t::FOREGROUND, true); + job->promote(); + job->set_flag(job_flag_t::FOREGROUND, true); - j->continue_job(j->is_stopped()); + job->continue_job(job->is_stopped()); return STATUS_CMD_OK; } diff --git a/src/builtin_jobs.cpp b/src/builtin_jobs.cpp index 6d9857992..c3673b89d 100644 --- a/src/builtin_jobs.cpp +++ b/src/builtin_jobs.cpp @@ -174,11 +174,9 @@ int builtin_jobs(parser_t &parser, io_streams_t &streams, wchar_t **argv) { if (print_last) { // Ignore unconstructed jobs, i.e. ourself. - job_iterator_t jobs; - const job_t *j; - while ((j = jobs.next())) { + for (auto j : jobs()) { if (j->is_constructed() && !j->is_completed()) { - builtin_jobs_print(j, mode, !streams.out_is_redirected, streams); + builtin_jobs_print(j.get(), mode, !streams.out_is_redirected, streams); return STATUS_CMD_ERROR; } } @@ -217,12 +215,10 @@ int builtin_jobs(parser_t &parser, io_streams_t &streams, wchar_t **argv) { } } } else { - job_iterator_t jobs; - const job_t *j; - while ((j = jobs.next())) { + for (auto j : jobs()) { // Ignore unconstructed jobs, i.e. ourself. if (j->is_constructed() && !j->is_completed()) { - builtin_jobs_print(j, mode, !found && !streams.out_is_redirected, streams); + builtin_jobs_print(j.get(), mode, !found && !streams.out_is_redirected, streams); found = true; } } diff --git a/src/builtin_wait.cpp b/src/builtin_wait.cpp index b517f7a78..34a687bb1 100644 --- a/src/builtin_wait.cpp +++ b/src/builtin_wait.cpp @@ -16,10 +16,7 @@ /// If a specified process has already finished but the job hasn't, parser_t::job_get_from_pid() /// doesn't work properly, so use this function in wait command. static job_id_t get_job_id_from_pid(pid_t pid) { - job_t *j; - job_iterator_t jobs; - - while ((j = jobs.next()) != nullptr) { + for (auto j : jobs()) { if (j->pgid == pid) { return j->job_id; } @@ -34,8 +31,7 @@ static job_id_t get_job_id_from_pid(pid_t pid) { } static bool all_jobs_finished() { - job_iterator_t jobs; - while (job_t *j = jobs.next()) { + for (auto j : jobs()) { // If any job is not completed, return false. // If there are stopped jobs, they are ignored. if (j->is_constructed() && !j->is_completed() && !j->is_stopped()) { @@ -46,14 +42,13 @@ static bool all_jobs_finished() { } static bool any_jobs_finished(size_t jobs_len) { - job_iterator_t jobs; bool no_jobs_running = true; // If any job is removed from list, return true. - if (jobs_len != jobs.count()) { + if (jobs_len != jobs().size()) { return true; } - while (job_t *j = jobs.next()) { + for (auto j : jobs()) { // If any job is completed, return true. if (j->is_constructed() && (j->is_completed() || j->is_stopped())) { return true; @@ -70,8 +65,7 @@ static bool any_jobs_finished(size_t jobs_len) { } static int wait_for_backgrounds(bool any_flag) { - job_iterator_t jobs; - size_t jobs_len = jobs.count(); + size_t jobs_len = jobs().size(); while ((!any_flag && !all_jobs_finished()) || (any_flag && !any_jobs_finished(jobs_len))) { if (reader_test_interrupted()) { @@ -143,10 +137,9 @@ static bool match_pid(const wcstring &cmd, const wchar_t *proc) { /// It should search the job list for something matching the given proc. static bool find_job_by_name(const wchar_t *proc, std::vector &ids) { - job_iterator_t jobs; bool found = false; - while (const job_t *j = jobs.next()) { + for (const auto j : jobs()) { if (j->command_is_empty()) continue; if (match_pid(j->command(), proc)) { @@ -179,7 +172,6 @@ static bool find_job_by_name(const wchar_t *proc, std::vector &ids) { int builtin_wait(parser_t &parser, io_streams_t &streams, wchar_t **argv) { ASSERT_IS_MAIN_THREAD(); int retval = STATUS_CMD_OK; - job_iterator_t jobs; const wchar_t *cmd = argv[0]; int argc = builtin_count_args(argv); bool any_flag = false; // flag for -n option diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 2034f0d56..8bd145c77 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -784,10 +784,8 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process( // Protect against exec with background processes running static uint32_t last_exec_run_counter = -1; if (process_type == process_type_t::exec && shell_is_interactive()) { - job_iterator_t jobs; bool have_bg = false; - const job_t *bg = nullptr; - while ((bg = jobs.next())) { + for (const auto bg : jobs()) { // The assumption here is that if it is a foreground job, // it's related to us. // This stops us from asking if we're doing `exec` inside a function. @@ -1130,6 +1128,16 @@ parse_execution_result_t parse_execution_context_t::populate_job_from_job_node( return result; } +static bool remove_job(job_t *job) { + for (auto j = jobs().begin(); j != jobs().end(); ++j) { + if (j->get() == job) { + jobs().erase(j); + return true; + } + } + return false; +} + parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t job_node, const block_t *associated_block) { if (should_cancel_execution(associated_block)) { @@ -1253,7 +1261,7 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t jo // Actually execute the job. if (!exec_job(*this->parser, job)) { - parser->job_remove(job.get()); + remove_job(job.get()); } // Only external commands require a new fishd barrier. diff --git a/src/parser.cpp b/src/parser.cpp index 15871ed03..2a94a6b8e 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -570,19 +570,6 @@ void parser_t::job_add(shared_ptr job) { this->my_job_list.push_front(std::move(job)); } -bool parser_t::job_remove(job_t *job) { - for (auto iter = my_job_list.begin(); iter != my_job_list.end(); ++iter) { - if (iter->get() == job) { - my_job_list.erase(iter); - return true; - } - } - - debug(1, _(L"Job inconsistency")); - sanity_lose(); - return false; -} - void parser_t::job_promote(job_t *job) { job_list_t::iterator loc; for (loc = my_job_list.begin(); loc != my_job_list.end(); ++loc) { @@ -604,20 +591,17 @@ job_t *parser_t::job_get(job_id_t id) { } job_t *parser_t::job_get_from_pid(pid_t pid) const { - job_iterator_t jobs; - job_t *job; - pid_t pgid = getpgid(pid); if (pgid == -1) { return 0; } - while ((job = jobs.next())) { + for (auto job : jobs()) { if (job->pgid == pgid) { for (const process_ptr_t &p : job->processes) { if (p->pid == pid) { - return job; + return job.get(); } } } diff --git a/src/parser.h b/src/parser.h index 264b09cc8..e6409c283 100644 --- a/src/parser.h +++ b/src/parser.h @@ -288,9 +288,6 @@ class parser_t : public std::enable_shared_from_this { /// Return the function name for the specified stack frame. Default is one (current frame). const wchar_t *get_function_name(int level = 1); - /// Removes a job. - bool job_remove(job_t *job); - /// Promotes a job to the front of the list. void job_promote(job_t *job); diff --git a/src/proc.cpp b/src/proc.cpp index 119bbfeee..2f0dc6b12 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -59,20 +59,11 @@ bool job_list_is_empty() { return parser_t::principal_parser().job_list().empty(); } -void job_iterator_t::reset() { - this->current = job_list->begin(); - this->end = job_list->end(); -} - -job_iterator_t::job_iterator_t(job_list_t &jobs) : job_list(&jobs) { this->reset(); } - -job_iterator_t::job_iterator_t() : job_list(&parser_t::principal_parser().job_list()) { +job_list_t& jobs() { ASSERT_IS_MAIN_THREAD(); - this->reset(); + return parser_t::principal_parser().job_list(); } -size_t job_iterator_t::count() const { return this->job_list->size(); } - bool is_interactive_session = false; bool is_subshell = false; bool is_block = false; @@ -110,23 +101,15 @@ static std::vector interactive_stack; void proc_init() { proc_push_interactive(0); } -/// Remove job from list of jobs. -static int job_remove(job_t *j) { - ASSERT_IS_MAIN_THREAD(); - return parser_t::principal_parser().job_remove(j); -} - void job_t::promote() { ASSERT_IS_MAIN_THREAD(); parser_t::principal_parser().job_promote(this); } void proc_destroy() { - job_list_t &jobs = parser_t::principal_parser().job_list(); - while (!jobs.empty()) { - job_t *job = jobs.front().get(); - debug(2, L"freeing leaked job %ls", job->command_wcstr()); - job_remove(job); + for (auto job = jobs().begin(); job != jobs().end(); ++job) { + debug(2, L"freeing leaked job %ls", (*job)->command_wcstr()); + job = jobs().erase(job); } } @@ -364,8 +347,7 @@ static void process_mark_finished_children(bool block_ok) { topic_set_t reaptopics{}; generation_list_t gens{}; gens.fill(invalid_generation); - job_iterator_t jobs; - while (auto *j = jobs.next()) { + for (const auto j: jobs()) { for (const auto &proc : j->processes) { if (auto mtopic = j->reap_topic_for_process(proc.get())) { topic_t topic = *mtopic; @@ -390,8 +372,7 @@ static void process_mark_finished_children(bool block_ok) { // We got some changes. Since we last checked we received SIGCHLD, and or HUP/INT. // Update the hup/int generations and reap any reapable processes. - jobs.reset(); - while (auto *j = jobs.next()) { + for (const auto j : jobs()) { for (const auto &proc : j->processes) { if (auto mtopic = j->reap_topic_for_process(proc.get())) { // Update the signal hup/int gen. @@ -484,7 +465,7 @@ static bool process_clean_after_marking(bool allow_interactive) { // avoid infinite recursion). static bool locked = false; if (locked) { - return 0; + return false; } locked = true; @@ -492,9 +473,10 @@ static bool process_clean_after_marking(bool allow_interactive) { // don't try to print in that case (#3222) const bool interactive = allow_interactive && cur_term != NULL; - job_iterator_t jobs; - const bool only_one_job = jobs.count() == 1; - while (job_t *const j = jobs.next()) { + bool erased = false; + const bool only_one_job = jobs().size() == 1; + for (auto itr = jobs().begin(); itr != jobs().end(); itr = (erased ? itr : (std::advance(itr, 1), itr)), erased = false) { + job_t *j = itr->get(); // If we are reaping only jobs who do not need status messages sent to the console, do not // consider reaping jobs that need status messages. if ((!j->get_flag(job_flag_t::SKIP_NOTIFICATION)) && (!interactive) && @@ -587,7 +569,8 @@ static bool process_clean_after_marking(bool allow_interactive) { } proc_fire_event(L"JOB_EXIT", event_type_t::job_exit, j->job_id, 0); - job_remove(j); + itr = jobs().erase(itr); + erased = true; } else if (j->is_stopped() && !j->get_flag(job_flag_t::NOTIFIED)) { // Notify the user about newly stopped jobs. if (!j->get_flag(job_flag_t::SKIP_NOTIFICATION)) { @@ -663,10 +646,7 @@ unsigned long proc_get_jiffies(process_t *p) { /// Update the CPU time for all jobs. void proc_update_jiffies() { - job_t *job; - job_iterator_t j; - - for (job = j.next(); job; job = j.next()) { + for (auto job : jobs()) { for (process_ptr_t &p : job->processes) { gettimeofday(&p->last_time, 0); p->last_jiffies = proc_get_jiffies(p.get()); @@ -901,8 +881,7 @@ void job_t::continue_job(bool send_sigcont) { void proc_sanity_check() { const job_t *fg_job = NULL; - job_iterator_t jobs; - while (const job_t *j = jobs.next()) { + for (const auto j : jobs()) { if (!j->is_constructed()) continue; // More than one foreground job? @@ -912,7 +891,7 @@ void proc_sanity_check() { fg_job->command_wcstr(), j->command_wcstr()); sanity_lose(); } - fg_job = j; + fg_job = j.get(); } for (const process_ptr_t &p : j->processes) { @@ -959,9 +938,7 @@ void proc_wait_any() { } void hup_background_jobs() { - job_iterator_t jobs; - - while (job_t *j = jobs.next()) { + for (auto j : jobs()) { // Make sure we don't try to SIGHUP the calling builtin if (j->pgid == INVALID_PID || !j->get_flag(job_flag_t::JOB_CONTROL)) { continue; diff --git a/src/proc.h b/src/proc.h index 716fd42f7..dbc899a44 100644 --- a/src/proc.h +++ b/src/proc.h @@ -456,27 +456,8 @@ typedef std::list> job_list_t; bool job_list_is_empty(void); -/// A class to aid iteration over jobs list -class job_iterator_t { - job_list_t *const job_list; - job_list_t::iterator current, end; - - public: - void reset(void); - - job_t *next() { - job_t *job = NULL; - if (current != end) { - job = current->get(); - ++current; - } - return job; - } - - explicit job_iterator_t(job_list_t &jobs); - job_iterator_t(); - size_t count() const; -}; +/// A helper function to more easily access the job list +job_list_t &jobs(); /// Whether a universal variable barrier roundtrip has already been made for the currently executing /// command. Such a roundtrip only needs to be done once on a given command, unless a universal diff --git a/src/reader.cpp b/src/reader.cpp index 409e69b46..e2c072700 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -2229,8 +2229,7 @@ void reader_bg_job_warning() { std::fputws(_(L"There are still jobs active:\n"), stdout); std::fputws(_(L"\n PID Command\n"), stdout); - job_iterator_t jobs; - while (job_t *j = jobs.next()) { + for (auto j : jobs()) { if (!j->is_completed()) { std::fwprintf(stdout, L"%6d %ls\n", j->processes[0]->pid, j->command_wcstr()); } @@ -2254,8 +2253,7 @@ static void handle_end_loop() { } bool bg_jobs = false; - job_iterator_t jobs; - while (const job_t *j = jobs.next()) { + for (const auto j : jobs()) { if (!j->is_completed()) { bg_jobs = true; break;