From aa477195d44b156cafaae098776ecddedfb9911e Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Wed, 19 Dec 2012 00:32:03 +0100 Subject: [PATCH 1/6] testing for bug #449 --- event.cpp | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 69 insertions(+), 3 deletions(-) diff --git a/event.cpp b/event.cpp index 8d6fdbc36..7f996eeed 100644 --- a/event.cpp +++ b/event.cpp @@ -25,6 +25,9 @@ #include "event.h" #include "signal.h" +FILE *checkfile; +FILE *tracefile; + /** Number of signals that can be queued before an overflow occurs */ @@ -131,6 +134,7 @@ static int event_match(const event_t *classv, const event_t *instance) /** This should never be reached */ + debug(1, "Warning: Unreachable code reached in event_match in event.cpp\n"); return 0; } @@ -216,7 +220,7 @@ wcstring event_get_desc(const event_t *e) break; default: - result = format_string(_(L"Unknown event type")); + result = format_string(_(L"Unknown event type '0x%x'"), e->type); break; } @@ -237,12 +241,60 @@ static void show_all_handlers(void) } #endif + +static wcstring event_type_str(const event_t *event) { + wcstring res; + char const *temp; + int sig; + switch(event->type) { + case EVENT_ANY: + res += L"EVENT_ANY"; + break; + case EVENT_VARIABLE: + if(event->str_param1.c_str()) { + res += format_string(L"EVENT_VARIABLE($%ls)", event->str_param1.c_str()); + } else { + res += L"EVENT_VARIABLE([any])"; + } + break; + case EVENT_SIGNAL: + sig = event->param1.signal; + if(sig == EVENT_ANY_SIGNAL) { + temp = "[all signals]"; + } else if(sig == 0) { + temp = "not set"; + } else { + temp = strsignal(sig); + } + res += format_string(L"EVENT_SIGNAL(%d: %s)", sig, temp); + break; + case EVENT_EXIT: + if(event->param1.pid == EVENT_ANY_PID) { + res += L"EVENT_EXIT([all child processes])"; + } else { + res += format_string(L"EVENT_EXIT(pid %d)", event->param1.pid); + } + break; + case EVENT_JOB_ID: + res += format_string(L"EVENT_JOB_ID(%d)", event->param1.job_id); + break; + case EVENT_GENERIC: + res += format_string(L"EVENT_GENERIC(%ls)", event->str_param1.c_str()); + break; + default: + res += format_string(L"unknown/illegal event(%x)", event->type); + } + return format_string(L"%ls: \"%ls\"", res.c_str(), event->function_name.c_str()); +} + + void event_add_handler(const event_t *event) { event_t *e; CHECK(event,); - + fprintf(tracefile, "register: %ls\n", event_type_str(event).c_str()); + e = event_copy(event, 0); if (e->type == EVENT_SIGNAL) @@ -263,6 +315,7 @@ void event_remove(event_t *criterion) event_list_t new_list; CHECK(criterion,); + fprintf(tracefile, "unregister: %ls\n", event_type_str(criterion).c_str()); /* Because of concurrency issues (env_remove could remove an event @@ -369,6 +422,13 @@ static int event_is_killed(event_t *e) return std::find(killme.begin(), killme.end(), e) != killme.end(); } +void event_print_all(FILE *f, const wcstring& header, const std::vector &events) { + fprintf(f, "%ls", header.c_str()); + for(uint i=0; i Date: Wed, 19 Dec 2012 15:56:19 +0100 Subject: [PATCH 2/6] fixed #449, added test --- event.cpp | 78 +++++++++++++++++++++++++--------------------- tests/test9.err | 0 tests/test9.in | 25 +++++++++++++++ tests/test9.out | 2 ++ tests/test9.status | 1 + 5 files changed, 70 insertions(+), 36 deletions(-) create mode 100644 tests/test9.err create mode 100644 tests/test9.in create mode 100644 tests/test9.out create mode 100644 tests/test9.status diff --git a/event.cpp b/event.cpp index 7f996eeed..e4f452e07 100644 --- a/event.cpp +++ b/event.cpp @@ -25,8 +25,6 @@ #include "event.h" #include "signal.h" -FILE *checkfile; -FILE *tracefile; /** Number of signals that can be queued before an overflow occurs @@ -134,7 +132,7 @@ static int event_match(const event_t *classv, const event_t *instance) /** This should never be reached */ - debug(1, "Warning: Unreachable code reached in event_match in event.cpp\n"); + debug(0, "Warning: Unreachable code reached in event_match in event.cpp\n"); return 0; } @@ -244,47 +242,63 @@ static void show_all_handlers(void) static wcstring event_type_str(const event_t *event) { wcstring res; - char const *temp; + wchar_t const *temp; int sig; switch(event->type) { case EVENT_ANY: - res += L"EVENT_ANY"; + res = L"EVENT_ANY"; break; case EVENT_VARIABLE: if(event->str_param1.c_str()) { - res += format_string(L"EVENT_VARIABLE($%ls)", event->str_param1.c_str()); + res = format_string(L"EVENT_VARIABLE($%ls)", event->str_param1.c_str()); } else { - res += L"EVENT_VARIABLE([any])"; + res = L"EVENT_VARIABLE([any])"; } break; case EVENT_SIGNAL: sig = event->param1.signal; if(sig == EVENT_ANY_SIGNAL) { - temp = "[all signals]"; + temp = L"[all signals]"; } else if(sig == 0) { - temp = "not set"; + temp = L"not set"; } else { - temp = strsignal(sig); + temp = sig2wcs(sig); } - res += format_string(L"EVENT_SIGNAL(%d: %s)", sig, temp); + res = format_string(L"EVENT_SIGNAL(%ls)", temp); break; case EVENT_EXIT: if(event->param1.pid == EVENT_ANY_PID) { - res += L"EVENT_EXIT([all child processes])"; + res = wcstring(L"EVENT_EXIT([all child processes])"); + } else if (event->param1.pid > 0) { + res = format_string(L"EVENT_EXIT(pid %d)", event->param1.pid); } else { - res += format_string(L"EVENT_EXIT(pid %d)", event->param1.pid); + job_t *j = job_get_from_pid(-event->param1.pid); + if (j) + res = format_string(L"EVENT_EXIT(jobid %d: \"%ls\")", j->job_id, j->command_wcstr()); + else + res = format_string(L"EVENT_EXIT(pgid %d)", -event->param1.pid); } break; case EVENT_JOB_ID: - res += format_string(L"EVENT_JOB_ID(%d)", event->param1.job_id); + { + job_t *j = job_get(event->param1.job_id); + if (j) + res = format_string(L"EVENT_JOB_ID(job %d: \"%ls\")", j->job_id, j->command_wcstr()); + else + res = format_string(L"EVENT_JOB_ID(jobid %d)", event->param1.job_id); break; + } case EVENT_GENERIC: - res += format_string(L"EVENT_GENERIC(%ls)", event->str_param1.c_str()); + res = format_string(L"EVENT_GENERIC(%ls)", event->str_param1.c_str()); break; default: - res += format_string(L"unknown/illegal event(%x)", event->type); + res = format_string(L"unknown/illegal event(%x)", event->type); + } + if(event->function_name.size()) { + return format_string(L"%ls: \"%ls\"", res.c_str(), event->function_name.c_str()); + } else { + return res; } - return format_string(L"%ls: \"%ls\"", res.c_str(), event->function_name.c_str()); } @@ -293,7 +307,8 @@ void event_add_handler(const event_t *event) event_t *e; CHECK(event,); - fprintf(tracefile, "register: %ls\n", event_type_str(event).c_str()); + if(debug_level >= 3) + debug(3, "register: %ls\n", event_type_str(event).c_str()); e = event_copy(event, 0); @@ -315,7 +330,8 @@ void event_remove(event_t *criterion) event_list_t new_list; CHECK(criterion,); - fprintf(tracefile, "unregister: %ls\n", event_type_str(criterion).c_str()); + if(debug_level >= 3) + debug(3, "unregister: %ls\n", event_type_str(criterion).c_str()); /* Because of concurrency issues (env_remove could remove an event @@ -422,13 +438,6 @@ static int event_is_killed(event_t *e) return std::find(killme.begin(), killme.end(), e) != killme.end(); } -void event_print_all(FILE *f, const wcstring& header, const std::vector &events) { - fprintf(f, "%ls", header.c_str()); - for(uint i=0; i Date: Thu, 20 Dec 2012 01:11:55 +0100 Subject: [PATCH 3/6] Allow 'emit' to accept event arguments --- builtin.cpp | 10 +++++----- doc_src/emit.txt | 9 +++++---- event.cpp | 17 +++-------------- event.h | 4 +--- parser.cpp | 5 ++++- tests/test9.out | 2 +- 6 files changed, 19 insertions(+), 28 deletions(-) diff --git a/builtin.cpp b/builtin.cpp index f8eec3f23..a25110f64 100644 --- a/builtin.cpp +++ b/builtin.cpp @@ -1010,15 +1010,15 @@ static int builtin_emit(parser_t &parser, wchar_t **argv) } - for (; woptind < argc; woptind++) + wcstring_list_t args; + wchar_t *eventname = argv[woptind]; + for (woptind++; woptind < argc; woptind++) { - event_fire_generic(argv[woptind]); + args.push_back(argv[woptind]); } + event_fire_generic(eventname, &args); return STATUS_BUILTIN_OK; - - - } diff --git a/doc_src/emit.txt b/doc_src/emit.txt index f28217487..c0faab82f 100644 --- a/doc_src/emit.txt +++ b/doc_src/emit.txt @@ -1,11 +1,11 @@ \section emit emit - Emit a generic event \subsection block-synopsis Synopsis - emit EVENT_NAME + emit EVENT_NAME [ARGUMENTS...] \subsection emit-description Description -The emit builtin fires a generic fish event. Such events can be caught by special functions called event handlers. +The emit builtin fires a generic fish event. Such events can be caught by special functions called event handlers. The arguments are passed to the event handlers as function arguments. \subsection emit-example Example @@ -13,7 +13,8 @@ The following code first defines an event handler for the generic event named 'test_event', and then emits an event of that type. 
function event_test --on-event test_event
-    echo event test!!!
+    echo event test: $argv
 end
 
-emit test_event
\ No newline at end of file +emit test_event something + diff --git a/event.cpp b/event.cpp index e4f452e07..8c61ebd00 100644 --- a/event.cpp +++ b/event.cpp @@ -689,26 +689,15 @@ void event_free(event_t *e) delete e; } - -void event_fire_generic_internal(const wchar_t *name, ...) +void event_fire_generic(const wchar_t *name, wcstring_list_t *args) { - va_list va; - wchar_t *arg; - CHECK(name,); event_t ev(EVENT_GENERIC); ev.str_param1 = name; - ev.arguments.reset(new wcstring_list_t); - va_start(va, name); - while ((arg=va_arg(va, wchar_t *))!= 0) - { - ev.arguments->push_back(arg); - } - va_end(va); - + if (args) + ev.arguments.reset(new wcstring_list_t(*args)); event_fire(&ev); - ev.arguments.reset(NULL); } event_t event_t::signal_event(int sig) diff --git a/event.h b/event.h index 034dafab2..56d076bea 100644 --- a/event.h +++ b/event.h @@ -173,8 +173,6 @@ wcstring event_get_desc(const event_t *e); /** Fire a generic event with the specified name */ -#define event_fire_generic( ... ) event_fire_generic_internal( __VA_ARGS__, NULL ) - -void event_fire_generic_internal(const wchar_t *name,...); +void event_fire_generic(const wchar_t *name, wcstring_list_t *args = NULL); #endif diff --git a/parser.cpp b/parser.cpp index 0f8950f05..f022f3eae 100644 --- a/parser.cpp +++ b/parser.cpp @@ -2077,6 +2077,7 @@ int parser_t::parse_job(process_t *p, int tmp; const wchar_t *cmd = args.at(0).completion.c_str(); + wcstring_list_t event_args; /* We couldn't find the specified command. @@ -2157,7 +2158,9 @@ int parser_t::parse_job(process_t *p, current_tokenizer_pos=tmp; job_set_flag(j, JOB_SKIP, 1); - event_fire_generic(L"fish_command_not_found", (wchar_t *)(args.at(0).completion.c_str())); + + event_args.push_back(args.at(0).completion); + event_fire_generic(L"fish_command_not_found", &event_args); proc_set_last_status(err==ENOENT?STATUS_UNKNOWN_COMMAND:STATUS_NOT_EXECUTABLE); } } diff --git a/tests/test9.out b/tests/test9.out index 863e82f59..779f9a844 100644 --- a/tests/test9.out +++ b/tests/test9.out @@ -1,2 +1,2 @@ before:test1 -received event test3 with args: +received event test3 with args: foo bar From 71233ee89476aa92782ac8288eba6c1485a8477e Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Thu, 20 Dec 2012 10:52:44 +0100 Subject: [PATCH 4/6] Make event_t.arguments into a vector instead of an auto_ptr. Yay for less indirection and less code! The resulting event_t structure is two pointers larger, but cuts out an indirection and allocation. --- env.cpp | 24 +++++++++--------------- event.cpp | 55 +++++++++++++------------------------------------------ event.h | 7 ++++--- proc.cpp | 10 ++++------ 4 files changed, 30 insertions(+), 66 deletions(-) diff --git a/env.cpp b/env.cpp index 035fee4ec..183144c7f 100644 --- a/env.cpp +++ b/env.cpp @@ -415,12 +415,10 @@ static void universal_callback(fish_message_type_t type, mark_changed_exported(); event_t ev = event_t::variable_event(name); - ev.arguments.reset(new wcstring_list_t()); - ev.arguments->push_back(L"VARIABLE"); - ev.arguments->push_back(str); - ev.arguments->push_back(name); + ev.arguments.push_back(L"VARIABLE"); + ev.arguments.push_back(str); + ev.arguments.push_back(name); event_fire(&ev); - ev.arguments.reset(NULL); } if (name) @@ -979,15 +977,13 @@ int env_set(const wcstring &key, const wchar_t *val, int var_mode) if (!is_universal) { event_t ev = event_t::variable_event(key); - ev.arguments.reset(new wcstring_list_t); - ev.arguments->push_back(L"VARIABLE"); - ev.arguments->push_back(L"SET"); - ev.arguments->push_back(key); + ev.arguments.push_back(L"VARIABLE"); + ev.arguments.push_back(L"SET"); + ev.arguments.push_back(key); // debug( 1, L"env_set: fire events on variable %ls", key ); event_fire(&ev); // debug( 1, L"env_set: return from event firing" ); - ev.arguments.reset(NULL); } react_to_variable_change(key); @@ -1066,14 +1062,12 @@ int env_remove(const wcstring &key, int var_mode) if (try_remove(first_node, key.c_str(), var_mode)) { event_t ev = event_t::variable_event(key); - ev.arguments.reset(new wcstring_list_t); - ev.arguments->push_back(L"VARIABLE"); - ev.arguments->push_back(L"ERASE"); - ev.arguments->push_back(key); + ev.arguments.push_back(L"VARIABLE"); + ev.arguments.push_back(L"ERASE"); + ev.arguments.push_back(key); event_fire(&ev); - ev.arguments.reset(NULL); erased = 1; } } diff --git a/event.cpp b/event.cpp index 8c61ebd00..58a2f4fd0 100644 --- a/event.cpp +++ b/event.cpp @@ -137,22 +137,6 @@ static int event_match(const event_t *classv, const event_t *instance) } -/** - Create an identical copy of an event. Use deep copying, i.e. make - duplicates of any strings used as well. -*/ -static event_t *event_copy(const event_t *event, int copy_arguments) -{ - event_t *e = new event_t(*event); - - e->arguments.reset(new wcstring_list_t); - if (copy_arguments && event->arguments.get() != NULL) - { - *(e->arguments) = *(event->arguments); - } - - return e; -} /** Test if specified event is blocked @@ -307,10 +291,10 @@ void event_add_handler(const event_t *event) event_t *e; CHECK(event,); - if(debug_level >= 3) - debug(3, "register: %ls\n", event_type_str(event).c_str()); - - e = event_copy(event, 0); + if(debug_level >= 3) + debug(3, "register: %ls\n", event_type_str(event).c_str()); + + e = new event_t(*event); if (e->type == EVENT_SIGNAL) { @@ -506,11 +490,11 @@ static void event_fire_internal(const event_t *event) */ wcstring buffer = criterion->function_name; - if (event->arguments.get()) + if (! event->arguments.empty()) { - for (j=0; j< event->arguments->size(); j++) + for (j=0; j < event->arguments.size(); j++) { - wcstring arg_esc = escape_string(event->arguments->at(j), 1); + wcstring arg_esc = escape_string(event->arguments.at(j), 1); buffer += L" "; buffer += arg_esc; } @@ -566,7 +550,7 @@ static void event_fire_delayed() event_t *e = blocked.at(i); if (event_is_blocked(e)) { - new_blocked.push_back(e); + new_blocked.push_back(new event_t(*e)); } else { @@ -592,7 +576,6 @@ static void event_fire_delayed() Set up */ event_t e = event_t::signal_event(0); - e.arguments.reset(new wcstring_list_t(1)); //one element lst = &sig_list[1-active_list]; if (lst->overflow) @@ -606,10 +589,10 @@ static void event_fire_delayed() for (int i=0; i < lst->count; i++) { e.param1.signal = lst->signal[i]; - e.arguments->at(0) = sig2wcs(e.param1.signal); + e.arguments.at(0) = sig2wcs(e.param1.signal); if (event_is_blocked(&e)) { - blocked.push_back(event_copy(&e, 1)); + blocked.push_back(new event_t(e)); } else { @@ -617,8 +600,6 @@ static void event_fire_delayed() } } - e.arguments.reset(NULL); - } } @@ -657,7 +638,7 @@ void event_fire(event_t *event) { if (event_is_blocked(event)) { - blocked.push_back(event_copy(event, 1)); + blocked.push_back(new event_t(*event)); } else { @@ -696,7 +677,7 @@ void event_fire_generic(const wchar_t *name, wcstring_list_t *args) event_t ev(EVENT_GENERIC); ev.str_param1 = name; if (args) - ev.arguments.reset(new wcstring_list_t(*args)); + ev.arguments = *args; event_fire(&ev); } @@ -720,14 +701,4 @@ event_t event_t::generic_event(const wcstring &str) event.str_param1 = str; return event; } - -event_t::event_t(const event_t &x) : - type(x.type), - param1(x.param1), - str_param1(x.str_param1), - function_name(x.function_name) -{ - const wcstring_list_t *ptr = x.arguments.get(); - if (ptr) - arguments.reset(new wcstring_list_t(*ptr)); -} + diff --git a/event.h b/event.h index 56d076bea..283a54fb9 100644 --- a/event.h +++ b/event.h @@ -84,12 +84,13 @@ struct event_t event_fire. In all other situations, the value of this variable is ignored. */ - std::auto_ptr arguments; + wcstring_list_t arguments; event_t(int t) : type(t), param1(), str_param1(), function_name(), arguments() { } - /** Copy constructor */ - event_t(const event_t &x); + /** default copy constructor */ + //event_t(const event_t &x); + static event_t signal_event(int sig); static event_t variable_event(const wcstring &str); diff --git a/proc.cpp b/proc.cpp index b061bdd82..039c5ac1e 100644 --- a/proc.cpp +++ b/proc.cpp @@ -162,7 +162,6 @@ static std::vector interactive_stack; void proc_init() { proc_push_interactive(0); - event.arguments.reset(new wcstring_list_t); } @@ -194,7 +193,6 @@ void job_free(job_t * j) void proc_destroy() { - event.arguments.reset(NULL); job_list_t &jobs = parser_t::principal_parser().job_list(); while (! jobs.empty()) { @@ -603,11 +601,11 @@ void proc_fire_event(const wchar_t *msg, int type, pid_t pid, int status) event.type=type; event.param1.pid = pid; - event.arguments->push_back(msg); - event.arguments->push_back(to_string(pid)); - event.arguments->push_back(to_string(status)); + event.arguments.push_back(msg); + event.arguments.push_back(to_string(pid)); + event.arguments.push_back(to_string(status)); event_fire(&event); - event.arguments->resize(0); + event.arguments.resize(0); } int job_reap(bool interactive) From 30392bf66a79b91ad2fa10b369f151540c40c09e Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Thu, 20 Dec 2012 11:48:36 +0100 Subject: [PATCH 5/6] reference'ize event.cpp/h --- builtin.cpp | 2 +- event.cpp | 142 ++++++++++++++++++++++++--------------------------- event.h | 14 +++-- function.cpp | 4 +- parser.cpp | 2 +- 5 files changed, 82 insertions(+), 82 deletions(-) diff --git a/builtin.cpp b/builtin.cpp index a25110f64..30421de3d 100644 --- a/builtin.cpp +++ b/builtin.cpp @@ -1100,7 +1100,7 @@ static void functions_def(const wcstring &name, wcstring &out) search.function_name = name; std::vector ev; - event_get(&search, &ev); + event_get(search, &ev); out.append(L"function "); diff --git a/event.cpp b/event.cpp index 58a2f4fd0..196296a0d 100644 --- a/event.cpp +++ b/event.cpp @@ -87,45 +87,45 @@ static event_list_t blocked; they must name the same function. */ -static int event_match(const event_t *classv, const event_t *instance) +static int event_match(const event_t &classv, const event_t &instance) { /* If the function names are both non-empty and different, then it's not a match */ - if (! classv->function_name.empty() && - ! instance->function_name.empty() && - classv->function_name != instance->function_name) + if (! classv.function_name.empty() && + ! instance.function_name.empty() && + classv.function_name != instance.function_name) { return 0; } - if (classv->type == EVENT_ANY) + if (classv.type == EVENT_ANY) return 1; - if (classv->type != instance->type) + if (classv.type != instance.type) return 0; - switch (classv->type) + switch (classv.type) { case EVENT_SIGNAL: - if (classv->param1.signal == EVENT_ANY_SIGNAL) + if (classv.param1.signal == EVENT_ANY_SIGNAL) return 1; - return classv->param1.signal == instance->param1.signal; + return classv.param1.signal == instance.param1.signal; case EVENT_VARIABLE: - return instance->str_param1 == classv->str_param1; + return instance.str_param1 == classv.str_param1; case EVENT_EXIT: - if (classv->param1.pid == EVENT_ANY_PID) + if (classv.param1.pid == EVENT_ANY_PID) return 1; - return classv->param1.pid == instance->param1.pid; + return classv.param1.pid == instance.param1.pid; case EVENT_JOB_ID: - return classv->param1.job_id == instance->param1.job_id; + return classv.param1.job_id == instance.param1.job_id; case EVENT_GENERIC: - return instance->str_param1 == classv->str_param1; + return instance.str_param1 == classv.str_param1; } @@ -141,68 +141,66 @@ static int event_match(const event_t *classv, const event_t *instance) /** Test if specified event is blocked */ -static int event_is_blocked(event_t *e) +static int event_is_blocked(const event_t &e) { block_t *block; parser_t &parser = parser_t::principal_parser(); for (block = parser.current_block; block; block = block->outer) { - if (event_block_list_blocks_type(block->event_blocks, e->type)) + if (event_block_list_blocks_type(block->event_blocks, e.type)) return true; } - return event_block_list_blocks_type(parser.global_event_blocks, e->type); + return event_block_list_blocks_type(parser.global_event_blocks, e.type); } -wcstring event_get_desc(const event_t *e) +wcstring event_get_desc(const event_t &e) { - CHECK(e, 0); - wcstring result; - switch (e->type) + switch (e.type) { case EVENT_SIGNAL: - result = format_string(_(L"signal handler for %ls (%ls)"), sig2wcs(e->param1.signal), signal_get_desc(e->param1.signal)); + result = format_string(_(L"signal handler for %ls (%ls)"), sig2wcs(e.param1.signal), signal_get_desc(e.param1.signal)); break; case EVENT_VARIABLE: - result = format_string(_(L"handler for variable '%ls'"), e->str_param1.c_str()); + result = format_string(_(L"handler for variable '%ls'"), e.str_param1.c_str()); break; case EVENT_EXIT: - if (e->param1.pid > 0) + if (e.param1.pid > 0) { - result = format_string(_(L"exit handler for process %d"), e->param1.pid); + result = format_string(_(L"exit handler for process %d"), e.param1.pid); } else { - job_t *j = job_get_from_pid(-e->param1.pid); + job_t *j = job_get_from_pid(-e.param1.pid); if (j) result = format_string(_(L"exit handler for job %d, '%ls'"), j->job_id, j->command_wcstr()); else - result = format_string(_(L"exit handler for job with process group %d"), -e->param1.pid); + result = format_string(_(L"exit handler for job with process group %d"), -e.param1.pid); } break; case EVENT_JOB_ID: { - job_t *j = job_get(e->param1.job_id); + job_t *j = job_get(e.param1.job_id); if (j) result = format_string(_(L"exit handler for job %d, '%ls'"), j->job_id, j->command_wcstr()); else - result = format_string(_(L"exit handler for job with job id %d"), e->param1.job_id); + result = format_string(_(L"exit handler for job with job id %d"), e.param1.job_id); break; } case EVENT_GENERIC: - result = format_string(_(L"handler for generic event '%ls'"), e->str_param1.c_str()); + result = format_string(_(L"handler for generic event '%ls'"), e.str_param1.c_str()); break; default: - result = format_string(_(L"Unknown event type '0x%x'"), e->type); + result = format_string(_(L"Unknown event type '0x%x'"), e.type); break; } @@ -223,24 +221,24 @@ static void show_all_handlers(void) } #endif - -static wcstring event_type_str(const event_t *event) { + +wcstring event_type_str(const event_t &event) { wcstring res; wchar_t const *temp; int sig; - switch(event->type) { + switch(event.type) { case EVENT_ANY: res = L"EVENT_ANY"; break; case EVENT_VARIABLE: - if(event->str_param1.c_str()) { - res = format_string(L"EVENT_VARIABLE($%ls)", event->str_param1.c_str()); + if(event.str_param1.c_str()) { + res = format_string(L"EVENT_VARIABLE($%ls)", event.str_param1.c_str()); } else { res = L"EVENT_VARIABLE([any])"; } break; case EVENT_SIGNAL: - sig = event->param1.signal; + sig = event.param1.signal; if(sig == EVENT_ANY_SIGNAL) { temp = L"[all signals]"; } else if(sig == 0) { @@ -251,50 +249,49 @@ static wcstring event_type_str(const event_t *event) { res = format_string(L"EVENT_SIGNAL(%ls)", temp); break; case EVENT_EXIT: - if(event->param1.pid == EVENT_ANY_PID) { + if(event.param1.pid == EVENT_ANY_PID) { res = wcstring(L"EVENT_EXIT([all child processes])"); - } else if (event->param1.pid > 0) { - res = format_string(L"EVENT_EXIT(pid %d)", event->param1.pid); + } else if (event.param1.pid > 0) { + res = format_string(L"EVENT_EXIT(pid %d)", event.param1.pid); } else { - job_t *j = job_get_from_pid(-event->param1.pid); + job_t *j = job_get_from_pid(-event.param1.pid); if (j) res = format_string(L"EVENT_EXIT(jobid %d: \"%ls\")", j->job_id, j->command_wcstr()); else - res = format_string(L"EVENT_EXIT(pgid %d)", -event->param1.pid); + res = format_string(L"EVENT_EXIT(pgid %d)", -event.param1.pid); } break; case EVENT_JOB_ID: { - job_t *j = job_get(event->param1.job_id); + job_t *j = job_get(event.param1.job_id); if (j) res = format_string(L"EVENT_JOB_ID(job %d: \"%ls\")", j->job_id, j->command_wcstr()); else - res = format_string(L"EVENT_JOB_ID(jobid %d)", event->param1.job_id); + res = format_string(L"EVENT_JOB_ID(jobid %d)", event.param1.job_id); break; } case EVENT_GENERIC: - res = format_string(L"EVENT_GENERIC(%ls)", event->str_param1.c_str()); + res = format_string(L"EVENT_GENERIC(%ls)", event.str_param1.c_str()); break; default: - res = format_string(L"unknown/illegal event(%x)", event->type); + res = format_string(L"unknown/illegal event(%x)", event.type); } - if(event->function_name.size()) { - return format_string(L"%ls: \"%ls\"", res.c_str(), event->function_name.c_str()); + if(event.function_name.size()) { + return format_string(L"%ls: \"%ls\"", res.c_str(), event.function_name.c_str()); } else { return res; } } -void event_add_handler(const event_t *event) +void event_add_handler(const event_t &event) { event_t *e; - CHECK(event,); if(debug_level >= 3) debug(3, "register: %ls\n", event_type_str(event).c_str()); - e = new event_t(*event); + e = new event_t(event); if (e->type == EVENT_SIGNAL) { @@ -307,13 +304,12 @@ void event_add_handler(const event_t *event) signal_unblock(); } -void event_remove(event_t *criterion) +void event_remove(event_t &criterion) { size_t i; event_list_t new_list; - CHECK(criterion,); if(debug_level >= 3) debug(3, "unregister: %ls\n", event_type_str(criterion).c_str()); @@ -332,7 +328,7 @@ void event_remove(event_t *criterion) for (i=0; itype == EVENT_SIGNAL) { event_t e = event_t::signal_event(n->param1.signal); - if (event_get(&e, 0) == 1) + if (event_get(e, 0) == 1) { signal_handle(e.param1.signal, 0); } @@ -360,7 +356,7 @@ void event_remove(event_t *criterion) signal_unblock(); } -int event_get(event_t *criterion, std::vector *out) +int event_get(const event_t &criterion, std::vector *out) { size_t i; int found = 0; @@ -368,12 +364,10 @@ int event_get(event_t *criterion, std::vector *out) if (events.empty()) return 0; - CHECK(criterion, 0); - for (i=0; ifunction_name; - if (! event->arguments.empty()) + if (! event.arguments.empty()) { - for (j=0; j < event->arguments.size(); j++) + for (j=0; j < event.arguments.size(); j++) { - wcstring arg_esc = escape_string(event->arguments.at(j), 1); + wcstring arg_esc = escape_string(event.arguments.at(j), 1); buffer += L" "; buffer += arg_esc; } @@ -510,7 +504,7 @@ static void event_fire_internal(const event_t *event) prev_status = proc_get_last_status(); parser_t &parser = parser_t::principal_parser(); - block_t *block = new event_block_t(event); + block_t *block = new event_block_t((const event_t*)&event); parser.push_block(block); parser.eval(buffer, io_chain_t(), TOP); parser.pop_block(); @@ -548,13 +542,13 @@ static void event_fire_delayed() for (i=0; isignal[i]; e.arguments.at(0) = sig2wcs(e.param1.signal); - if (event_is_blocked(&e)) + if (event_is_blocked(e)) { blocked.push_back(new event_t(e)); } else { - event_fire_internal(&e); + event_fire_internal(e); } } @@ -636,13 +630,13 @@ void event_fire(event_t *event) if (event) { - if (event_is_blocked(event)) + if (event_is_blocked(*event)) { blocked.push_back(new event_t(*event)); } else { - event_fire_internal(event); + event_fire_internal(*event); } } is_event--; diff --git a/event.h b/event.h index 283a54fb9..1c87488e3 100644 --- a/event.h +++ b/event.h @@ -102,14 +102,14 @@ struct event_t May not be called by a signal handler, since it may allocate new memory. */ -void event_add_handler(const event_t *event); +void event_add_handler(const event_t &event); /** Remove all events matching the specified criterion. May not be called by a signal handler, since it may free allocated memory. */ -void event_remove(event_t *event); +void event_remove(event_t &event); /** Return all events which match the specified event class @@ -122,7 +122,7 @@ void event_remove(event_t *event); \return the number of found matches */ -int event_get(event_t *criterion, std::vector *out); +int event_get(const event_t &criterion, std::vector *out); /** Returns whether an event listener is registered for the given signal. @@ -169,7 +169,13 @@ void event_free(event_t *e); /** Returns a string describing the specified event. */ -wcstring event_get_desc(const event_t *e); +wcstring event_get_desc(const event_t &e); + +/** + Returns a string describing the event. This version is more concise and + more detailed than event_get_desc. +*/ +wcstring event_type_str(const event_t &e); /** Fire a generic event with the specified name diff --git a/function.cpp b/function.cpp index 99e6218f5..08fb85560 100644 --- a/function.cpp +++ b/function.cpp @@ -199,7 +199,7 @@ void function_add(const function_data_t &data, const parser_t &parser) /* Add event handlers */ for (std::vector::const_iterator iter = data.events.begin(); iter != data.events.end(); ++iter) { - event_add_handler(&*iter); + event_add_handler(*iter); } } @@ -229,7 +229,7 @@ static bool function_remove_ignore_autoload(const wcstring &name) { event_t ev(EVENT_ANY); ev.function_name=name; - event_remove(&ev); + event_remove(ev); } return erased; diff --git a/parser.cpp b/parser.cpp index f022f3eae..1fe7bd83e 100644 --- a/parser.cpp +++ b/parser.cpp @@ -893,7 +893,7 @@ void parser_t::stack_trace(block_t *b, wcstring &buff) This is an event handler */ const event_block_t *eb = static_cast(b); - wcstring description = event_get_desc(eb->event); + wcstring description = event_get_desc(*eb->event); append_format(buff, _(L"in event handler: %ls\n"), description.c_str()); buff.append(L"\n"); From 8a446f43ff184fad6f7aeca05fca51bb3aac2d91 Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Sat, 22 Dec 2012 18:38:28 +0100 Subject: [PATCH 6/6] include fixes and suggestions from code review --- builtin.cpp | 10 +++++----- event.cpp | 24 ++++++++++++++++-------- event.h | 8 +------- parser.cpp | 4 ++-- parser.h | 4 ++-- tests/test9.err | 1 + tests/test9.in | 3 +++ tests/test9.status | 2 +- 8 files changed, 31 insertions(+), 25 deletions(-) diff --git a/builtin.cpp b/builtin.cpp index 30421de3d..7df552e26 100644 --- a/builtin.cpp +++ b/builtin.cpp @@ -1010,12 +1010,12 @@ static int builtin_emit(parser_t &parser, wchar_t **argv) } - wcstring_list_t args; - wchar_t *eventname = argv[woptind]; - for (woptind++; woptind < argc; woptind++) - { - args.push_back(argv[woptind]); + if(!argv[woptind]) { + append_format(stderr_buffer, L"%ls: expected event name\n", argv[0]); + return STATUS_BUILTIN_ERROR; } + wchar_t *eventname = argv[woptind]; + wcstring_list_t args(argv + woptind + 1, argv + argc); event_fire_generic(eventname, &args); return STATUS_BUILTIN_OK; diff --git a/event.cpp b/event.cpp index 196296a0d..496eb337e 100644 --- a/event.cpp +++ b/event.cpp @@ -221,8 +221,11 @@ static void show_all_handlers(void) } #endif - -wcstring event_type_str(const event_t &event) { +/* + Give a more condensed description of \c event compared to \c event_get_desc. + It includes what function will fire if the \c event is an event handler. + */ +static wcstring event_desc_compact(const event_t &event) { wcstring res; wchar_t const *temp; int sig; @@ -288,8 +291,10 @@ void event_add_handler(const event_t &event) { event_t *e; - if(debug_level >= 3) - debug(3, "register: %ls\n", event_type_str(event).c_str()); + if(debug_level >= 3) { + wcstring desc = event_desc_compact(event); + debug(3, "register: %ls\n", desc.c_str()); + } e = new event_t(event); @@ -304,14 +309,16 @@ void event_add_handler(const event_t &event) signal_unblock(); } -void event_remove(event_t &criterion) +void event_remove(const event_t &criterion) { size_t i; event_list_t new_list; - if(debug_level >= 3) - debug(3, "unregister: %ls\n", event_type_str(criterion).c_str()); + if(debug_level >= 3) { + wcstring desc = event_desc_compact(criterion); + debug(3, "unregister: %ls\n", desc.c_str()); + } /* Because of concurrency issues (env_remove could remove an event @@ -504,7 +511,7 @@ static void event_fire_internal(const event_t &event) prev_status = proc_get_last_status(); parser_t &parser = parser_t::principal_parser(); - block_t *block = new event_block_t((const event_t*)&event); + block_t *block = new event_block_t(event); parser.push_block(block); parser.eval(buffer, io_chain_t(), TOP); parser.pop_block(); @@ -570,6 +577,7 @@ static void event_fire_delayed() Set up */ event_t e = event_t::signal_event(0); + e.arguments.resize(1); lst = &sig_list[1-active_list]; if (lst->overflow) diff --git a/event.h b/event.h index 1c87488e3..cd3bb0e31 100644 --- a/event.h +++ b/event.h @@ -109,7 +109,7 @@ void event_add_handler(const event_t &event); May not be called by a signal handler, since it may free allocated memory. */ -void event_remove(event_t &event); +void event_remove(const event_t &event); /** Return all events which match the specified event class @@ -171,12 +171,6 @@ void event_free(event_t *e); */ wcstring event_get_desc(const event_t &e); -/** - Returns a string describing the event. This version is more concise and - more detailed than event_get_desc. -*/ -wcstring event_type_str(const event_t &e); - /** Fire a generic event with the specified name */ diff --git a/parser.cpp b/parser.cpp index 1fe7bd83e..a8da7ba08 100644 --- a/parser.cpp +++ b/parser.cpp @@ -893,7 +893,7 @@ void parser_t::stack_trace(block_t *b, wcstring &buff) This is an event handler */ const event_block_t *eb = static_cast(b); - wcstring description = event_get_desc(*eb->event); + wcstring description = event_get_desc(eb->event); append_format(buff, _(L"in event handler: %ls\n"), description.c_str()); buff.append(L"\n"); @@ -3798,7 +3798,7 @@ if_block_t::if_block_t() : { } -event_block_t::event_block_t(const event_t *evt) : +event_block_t::event_block_t(const event_t &evt) : block_t(EVENT), event(evt) { diff --git a/parser.h b/parser.h index 751182c35..b956be4d5 100644 --- a/parser.h +++ b/parser.h @@ -158,8 +158,8 @@ struct if_block_t : public block_t struct event_block_t : public block_t { - const event_t * const event; - event_block_t(const event_t *evt); + event_t const &event; + event_block_t(const event_t &evt); }; struct function_block_t : public block_t diff --git a/tests/test9.err b/tests/test9.err index e69de29bb..f439e8dba 100644 --- a/tests/test9.err +++ b/tests/test9.err @@ -0,0 +1 @@ +emit: expected event name diff --git a/tests/test9.in b/tests/test9.in index 688dfb3f2..b88427278 100644 --- a/tests/test9.in +++ b/tests/test9.in @@ -23,3 +23,6 @@ function test3 --on-event test3 end emit test3 foo bar + +# test empty argument +emit \ No newline at end of file diff --git a/tests/test9.status b/tests/test9.status index 573541ac9..d00491fd7 100644 --- a/tests/test9.status +++ b/tests/test9.status @@ -1 +1 @@ -0 +1