Refactor env_universal_callbacks

Reduce some allocations and simplify how events are emitted.

src/env_dispatch.cpp

@@ -209,20 +209,16 @@ void env_dispatch_var_change(const wcstring &key, env_stack_t &vars) {
     s_var_dispatch_table->dispatch(key, vars);
 }
 
-/// Universal variable callback function. This function makes sure the proper events are triggered
-/// when an event occurs.
-static void universal_callback(env_stack_t *stack, const callback_data_t &cb) {
-    const wchar_t *op = cb.is_erase() ? L"ERASE" : L"SET";
-
-    env_dispatch_var_change(cb.key, *stack);
-
-    // TODO: eliminate this principal_parser. Need to rationalize how multiple threads work here.
-    event_fire(parser_t::principal_parser(), event_t::variable(cb.key, {L"VARIABLE", op, cb.key}));
-}
-
+// Trigger events due to a universal variable changing.
 void env_universal_callbacks(env_stack_t *stack, const callback_data_list_t &callbacks) {
     for (const callback_data_t &cb : callbacks) {
-        universal_callback(stack, cb);
+        env_dispatch_var_change(cb.key, *stack);
+
+        // TODO: eliminate this principal_parser. Need to rationalize how multiple threads work
+        // here.
+        event_t evt =
+            cb.is_erase() ? event_t::variable_erase(cb.key) : event_t::variable_set(cb.key);
+        event_fire(parser_t::principal_parser(), evt);
     }
 }
 

src/env_universal_common.cpp

@@ -339,7 +339,7 @@ void env_universal_t::generate_callbacks_and_update_exports(const var_table_t &n
         }
         if (existing == this->vars.end() || export_changed || value_changed) {
             // Value is set for the first time, or has changed.
-            callbacks.push_back(callback_data_t(key, new_entry.as_string()));
+            callbacks.push_back(callback_data_t(key, new_entry));
         }
     }
 }

src/env_universal_common.h

@@ -20,10 +20,10 @@ struct callback_data_t {
     wcstring key;
 
     // The value of the variable, or none if it is erased.
-    maybe_t<wcstring> val;
+    maybe_t<env_var_t> val;
 
     /// Construct from a key and maybe a value.
-    callback_data_t(wcstring k, maybe_t<wcstring> v) : key(std::move(k)), val(std::move(v)) {}
+    callback_data_t(wcstring k, maybe_t<env_var_t> v) : key(std::move(k)), val(std::move(v)) {}
 
     /// \return whether this callback represents an erased variable.
     bool is_erase() const { return !val.has_value(); }

src/event.cpp

@@ -520,21 +520,19 @@ event_description_t event_description_t::generic(wcstring str) {
 }
 
 // static
-event_t event_t::variable(wcstring name, wcstring_list_t args) {
+event_t event_t::variable_erase(wcstring name) {
     event_t evt{event_type_t::variable};
+    evt.arguments = {L"VARIABLE", L"ERASE", name};
     evt.desc.str_param1 = std::move(name);
-    evt.arguments = std::move(args);
     return evt;
 }
 
-// static
-event_t event_t::variable_erase(wcstring name) {
-    return event_t::variable(name, {L"VARIABLE", L"ERASE", name});
-}
-
 // static
 event_t event_t::variable_set(wcstring name) {
-    return event_t::variable(name, {L"VARIABLE", L"SET", name});
+    event_t evt{event_type_t::variable};
+    evt.arguments = {L"VARIABLE", L"SET", name};
+    evt.desc.str_param1 = std::move(name);
+    return evt;
 }
 
 // static

src/event.h

@@ -111,8 +111,6 @@ struct event_t {
 
     explicit event_t(event_type_t t) : desc(t) {}
 
-    /// Create an event_type_t::variable event.
-    static event_t variable(wcstring name, wcstring_list_t args);
     /// Create an event_type_t::variable event with the args for erasing a variable.
     static event_t variable_erase(wcstring name);
     /// Create an event_type_t::variable event with the args for setting a variable.

src/fish_tests.cpp

@@ -4036,9 +4036,9 @@ static void test_universal_callbacks() {
     // Should see exactly three changes.
     do_test(callbacks.size() == 3);
     do_test(callbacks.at(0).key == L"alpha");
-    do_test(callbacks.at(0).val == wcstring{L"2"});
+    do_test(callbacks.at(0).val->as_string() == L"2");
     do_test(callbacks.at(1).key == L"beta");
-    do_test(callbacks.at(1).val == wcstring{L"1"});
+    do_test(callbacks.at(1).val->as_string() == L"1");
     do_test(callbacks.at(2).key == L"delta");
     do_test(callbacks.at(2).val == none());
     system_assert("rm -Rf test/fish_uvars_test/");