Big cleanup of env_set. Changed var_table_t to use direct var_entry_t instead of pointers. Changed some ints to bools.

1 files changed, 150 insertions, 194 deletions

diff --git a/env.cpp b/env.cpp
index 4787827e..2c0ceb3d 100644
--- a/env.cpp
+++ b/env.cpp
@@ -97,7 +97,7 @@ struct var_entry_t
     var_entry_t() : exportv(false) { }
 };
 
-typedef std::map<wcstring, var_entry_t*> var_table_t;
+typedef std::map<wcstring, var_entry_t> var_table_t;
 
 bool g_log_forks = false;
 bool g_use_posix_spawn = false; //will usually be set to true
@@ -122,7 +122,7 @@ struct env_node_t
     /**
        Does this node contain any variables which are exported to subshells
     */
-    int exportv;
+    bool exportv;
 
     /**
       Pointer to next level
@@ -130,7 +130,7 @@ struct env_node_t
     struct env_node_t *next;
 
 
-    env_node_t() : new_scope(0), exportv(0), next(NULL) { }
+    env_node_t() : new_scope(0), exportv(false), next(NULL) { }
 };
 
 class variable_entry_t
@@ -201,17 +201,6 @@ static void mark_changed_exported()
 static wcstring dyn_var;
 
 /**
-   Variable used by env_get_names to communicate auxiliary information
-   to add_key_to_string_set
-*/
-static int get_names_show_exported;
-/**
-   Variable used by env_get_names to communicate auxiliary information
-   to add_key_to_string_set
-*/
-static int get_names_show_unexported;
-
-/**
    List of all locale variable names
 */
 static const wchar_t * const locale_variable[] =
@@ -678,7 +667,7 @@ void env_init(const struct config_paths_t *paths /* or NULL */)
         long shlvl_i = wcstol(shlvl_str.c_str(), NULL, 10);
         if (shlvl_i >= 0)
         {
-            nshlvl_str = format_string(L"%ld", 1 + shlvl_i);
+            nshlvl_str = to_string<long>(shlvl_i + 1);
         }
     }
     env_set(L"SHLVL", nshlvl_str.c_str(), ENV_GLOBAL | ENV_EXPORT);
@@ -707,17 +696,15 @@ void env_destroy()
     env_read_only.clear();
     env_electric.clear();
 
-
     var_table_t::iterator iter;
     for (iter = global->begin(); iter != global->end(); ++iter)
     {
-        var_entry_t *entry = iter->second;
-        if (entry->exportv)
+        const var_entry_t &entry = iter->second;
+        if (entry.exportv)
         {
             mark_changed_exported();
+            break;
         }
-
-        delete entry;
     }
 
     delete top;
@@ -752,14 +739,12 @@ static env_node_t *env_get_node(const wcstring &key)
 int env_set(const wcstring &key, const wchar_t *val, int var_mode)
 {
     ASSERT_IS_MAIN_THREAD();
-    env_node_t *node = NULL;
     bool has_changed_old = has_changed_exported;
     bool has_changed_new = false;
-    var_entry_t *e=0;
     int done=0;
-
+    
     int is_universal = 0;
-
+    
     if (val && contains(key, L"PWD", L"HOME"))
     {
         /* Canoncalize our path; if it changes, recurse and try again. */
@@ -770,204 +755,190 @@ int env_set(const wcstring &key, const wchar_t *val, int var_mode)
             return env_set(key, val_canonical.c_str(), var_mode);
         }
     }
-
+    
     if ((var_mode & ENV_USER) && is_read_only(key))
     {
         return ENV_PERM;
     }
-
+    
     if (key == L"umask")
     {
         wchar_t *end;
-
+        
         /*
-             Set the new umask
-             */
+         Set the new umask
+         */
         if (val && wcslen(val))
         {
             errno=0;
             long mask = wcstol(val, &end, 8);
-
+            
             if (!errno && (!*end) && (mask <= 0777) && (mask >= 0))
             {
                 umask(mask);
             }
         }
-        /*
-             Do not actually create a umask variable, on env_get, it will
-             be calculated dynamically
-             */
+        /* Do not actually create a umask variable, on env_get, it will be calculated dynamically */
         return 0;
     }
-
+    
     /*
-       Zero element arrays are internaly not coded as null but as this
-       placeholder string
-       */
+     Zero element arrays are internaly not coded as null but as this
+     placeholder string
+     */
     if (!val)
     {
         val = ENV_NULL;
     }
-
+    
     if (var_mode & ENV_UNIVERSAL)
     {
-        int exportv;
-
-        if (!(var_mode & ENV_EXPORT) &&
-                !(var_mode & ENV_UNEXPORT))
+        bool exportv;
+        if (var_mode & ENV_EXPORT)
         {
-            exportv = env_universal_get_export(key);
+            // export
+            exportv = true;
+        }
+        else if (var_mode & ENV_UNEXPORT)
+        {
+            // unexport
+            exportv = false;
         }
         else
         {
-            exportv = (var_mode & ENV_EXPORT);
+            // not changing the export
+            exportv = env_universal_get_export(key);
         }
-
         env_universal_set(key, val, exportv);
         is_universal = 1;
-
+        
     }
     else
     {
-
-        node = env_get_node(key);
-        if (node)
+        // Determine the node
+        
+        env_node_t *preexisting_node = env_get_node(key);
+        bool preexisting_entry_exportv = false;
+        if (preexisting_node != NULL)
         {
-            var_table_t::iterator result = node->env.find(key);
-            assert(result != node->env.end());
-            e  = result->second;
-
-            if (e->exportv)
+            var_table_t::const_iterator result = preexisting_node->env.find(key);
+            assert(result != preexisting_node->env.end());
+            const var_entry_t &entry = result->second;
+            if (entry.exportv)
             {
+                preexisting_entry_exportv = true;
                 has_changed_new = true;
             }
         }
-
-        if ((var_mode & ENV_LOCAL) ||
-                (var_mode & ENV_GLOBAL))
+        
+        env_node_t *node = NULL;
+        if (var_mode & ENV_GLOBAL)
+        {
+            node = global_env;
+        }
+        else if (var_mode & ENV_LOCAL)
+        {
+            node = top;
+        }
+        else if (preexisting_node != NULL)
         {
-            node = (var_mode & ENV_GLOBAL)?global_env:top;
+            node = preexisting_node;
+            
+            if ((var_mode & (ENV_EXPORT | ENV_UNEXPORT)) == 0)
+            {
+                // use existing entry's exportv
+                var_mode = preexisting_entry_exportv ? ENV_EXPORT : 0;
+            }
         }
         else
         {
-            if (node)
+            if (! get_proc_had_barrier())
             {
-                if (!(var_mode & ENV_EXPORT) &&
-                        !(var_mode & ENV_UNEXPORT))
-                {
-                    var_mode = e->exportv?ENV_EXPORT:0;
-                }
+                set_proc_had_barrier(true);
+                env_universal_barrier();
             }
-            else
+            
+            if (env_universal_get(key))
             {
-                if (! get_proc_had_barrier())
+                bool exportv;
+                if (var_mode & ENV_EXPORT)
                 {
-                    set_proc_had_barrier(true);
-                    env_universal_barrier();
+                    exportv = true;
                 }
-
-                if (env_universal_get(key))
+                else if (var_mode & ENV_UNEXPORT)
                 {
-                    int exportv;
-
-                    if (!(var_mode & ENV_EXPORT) &&
-                            !(var_mode & ENV_UNEXPORT))
-                    {
-                        exportv = env_universal_get_export(key);
-                    }
-                    else
-                    {
-                        exportv = (var_mode & ENV_EXPORT);
-                    }
-
-                    env_universal_set(key, val, exportv);
-                    is_universal = 1;
-
-                    done = 1;
-
+                    exportv = false;
                 }
                 else
                 {
-                    /*
-                               New variable with unspecified scope. The default
-                               scope is the innermost scope that is shadowing,
-                               which will be either the current function or the
-                               global scope.
-                               */
-                    node = top;
-                    while (node->next && !node->new_scope)
-                    {
-                        node = node->next;
-                    }
+                    exportv = env_universal_get_export(key);
+                }
+                
+                env_universal_set(key, val, exportv);
+                is_universal = 1;
+                
+                done = 1;
+                
+            }
+            else
+            {
+                /*
+                 New variable with unspecified scope. The default
+                 scope is the innermost scope that is shadowing,
+                 which will be either the current function or the
+                 global scope.
+                 */
+                node = top;
+                while (node->next && !node->new_scope)
+                {
+                    node = node->next;
                 }
             }
         }
-
+        
         if (!done)
         {
-            var_entry_t *old_entry = NULL;
-            var_table_t::iterator result = node->env.find(key);
-            if (result != node->env.end())
+            // Set the entry in the node
+            // Note that operator[] accesses the existing entry, or creates a new one
+            var_entry_t &entry = node->env[key];
+            if (entry.exportv)
             {
-                old_entry = result->second;
-                node->env.erase(result);
+                // this variable already existed, and was exported
+                has_changed_new = true;
             }
-
-            var_entry_t *entry = NULL;
-            if (old_entry)
+            entry.val = val;
+            if (var_mode & ENV_EXPORT)
             {
-                entry = old_entry;
-
-                if ((var_mode & ENV_EXPORT) || entry->exportv)
-                {
-                    entry->exportv = !!(var_mode & ENV_EXPORT);
-                    has_changed_new = true;
-                }
+                // the new variable is exported
+                entry.exportv = true;
+                node->exportv = true;
+                has_changed_new = true;
             }
             else
             {
-                entry = new var_entry_t;
-
-                if (var_mode & ENV_EXPORT)
-                {
-                    entry->exportv = 1;
-                    has_changed_new = true;
-                }
-                else
-                {
-                    entry->exportv = 0;
-                }
-
-            }
-
-            entry->val = val;
-            node->env[key] = entry;
-
-            if (entry->exportv)
-            {
-                node->exportv=1;
+                entry.exportv = false;
             }
-
+            
             if (has_changed_old || has_changed_new)
                 mark_changed_exported();
         }
-
+        
     }
-
+    
     if (!is_universal)
     {
         event_t ev = event_t::variable_event(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" );
     }
-
+    
     react_to_variable_change(key);
-
+    
     return 0;
 }
 
@@ -978,33 +949,27 @@ int env_set(const wcstring &key, const wchar_t *val, int var_mode)
 
    \return zero if the variable was not found, non-zero otherwise
 */
-static int try_remove(env_node_t *n,
-                      const wchar_t *key,
-                      int var_mode)
+static bool try_remove(env_node_t *n, const wchar_t *key, int var_mode)
 {
-    if (n == 0)
+    if (n == NULL)
     {
-        return 0;
+        return false;
     }
 
     var_table_t::iterator result = n->env.find(key);
     if (result != n->env.end())
     {
-        var_entry_t *v = result->second;
-
-        if (v->exportv)
+        if (result->second.exportv)
         {
             mark_changed_exported();
         }
-
         n->env.erase(result);
-        delete v;
-        return 1;
+        return true;
     }
 
     if (var_mode & ENV_LOCAL)
     {
-        return 0;
+        return false;
     }
 
     if (n->new_scope)
@@ -1116,7 +1081,6 @@ env_var_t env_get_string(const wcstring &key)
             /* Lock around a local region */
             scoped_lock lock(env_lock);
 
-            var_entry_t *res = NULL;
             env_node_t *env = top;
             wcstring result;
 
@@ -1124,18 +1088,15 @@ env_var_t env_get_string(const wcstring &key)
             {
                 var_table_t::iterator result =  env->env.find(key);
                 if (result != env->env.end())
-                    res = result->second;
-
-
-                if (res != NULL)
                 {
-                    if (res->val == ENV_NULL)
+                    const var_entry_t &res = result->second;
+                    if (res.val == ENV_NULL)
                     {
                         return env_var_t::missing_var();
                     }
                     else
                     {
-                        return res->val;
+                        return res.val;
                     }
                 }
 
@@ -1171,13 +1132,12 @@ env_var_t env_get_string(const wcstring &key)
     }
 }
 
-int env_exist(const wchar_t *key, int mode)
+bool env_exist(const wchar_t *key, int mode)
 {
-    var_entry_t *res;
     env_node_t *env;
     wchar_t *item=0;
 
-    CHECK(key, 0);
+    CHECK(key, false);
 
     /*
       Read only variables all exist, and they are all global. A local
@@ -1190,13 +1150,13 @@ int env_exist(const wchar_t *key, int mode)
             //Such variables are never exported
             if (mode & ENV_EXPORT)
             {
-                return 0;
+                return false;
             }
             else if (mode & ENV_UNEXPORT)
             {
-                return 1;
+                return true;
             }
-            return 1;
+            return true;
         }
     }
 
@@ -1210,18 +1170,18 @@ int env_exist(const wchar_t *key, int mode)
 
             if (result != env->env.end())
             {
-                res  = result->second;
+                const var_entry_t &res = result->second;
 
                 if (mode & ENV_EXPORT)
                 {
-                    return res->exportv == 1;
+                    return res.exportv;
                 }
                 else if (mode & ENV_UNEXPORT)
                 {
-                    return res->exportv == 0;
+                    return ! res.exportv;
                 }
 
-                return 1;
+                return true;
             }
 
             if (mode & ENV_LOCAL)
@@ -1330,12 +1290,12 @@ void env_pop()
         var_table_t::iterator iter;
         for (iter = killme->env.begin(); iter != killme->env.end(); ++iter)
         {
-            var_entry_t *entry = iter->second;
-            if (entry->exportv)
+            const var_entry_t &entry = iter->second;
+            if (entry.exportv)
             {
                 mark_changed_exported();
+                break;
             }
-            delete entry;
         }
 
         delete killme;
@@ -1355,18 +1315,18 @@ void env_pop()
 /**
    Function used with to insert keys of one table into a set::set<wcstring>
 */
-static void add_key_to_string_set(const var_table_t &envs, std::set<wcstring> &strSet)
+static void add_key_to_string_set(const var_table_t &envs, std::set<wcstring> *str_set, bool show_exported, bool show_unexported)
 {
     var_table_t::const_iterator iter;
     for (iter = envs.begin(); iter != envs.end(); ++iter)
     {
-        var_entry_t *e = iter->second;
+        const var_entry_t &e = iter->second;
 
-        if ((e->exportv && get_names_show_exported) ||
-                (!e->exportv && get_names_show_unexported))
+        if ((e.exportv && show_exported) ||
+                (!e.exportv && show_unexported))
         {
-            /*Insert Key*/
-            strSet.insert(iter->first);
+            /* Insert this key */
+            str_set->insert(iter->first);
         }
 
     }
@@ -1383,12 +1343,8 @@ wcstring_list_t env_get_names(int flags)
     int show_universal = flags & ENV_UNIVERSAL;
 
     env_node_t *n=top;
-
-
-    get_names_show_exported =
-        (flags & ENV_EXPORT) || !(flags & ENV_UNEXPORT);
-    get_names_show_unexported =
-        (flags & ENV_UNEXPORT) || !(flags & ENV_EXPORT);
+    const bool show_exported = (flags & ENV_EXPORT) || !(flags & ENV_UNEXPORT);
+    const bool show_unexported = (flags & ENV_UNEXPORT) || !(flags & ENV_EXPORT);
 
     if (!show_local && !show_global && !show_universal)
     {
@@ -1402,7 +1358,7 @@ wcstring_list_t env_get_names(int flags)
             if (n == global_env)
                 break;
 
-            add_key_to_string_set(n->env, names);
+            add_key_to_string_set(n->env, &names, show_exported, show_unexported);
             if (n->new_scope)
                 break;
             else
@@ -1413,13 +1369,13 @@ wcstring_list_t env_get_names(int flags)
 
     if (show_global)
     {
-        add_key_to_string_set(global_env->env, names);
-        if (get_names_show_unexported)
+        add_key_to_string_set(global_env->env, &names, show_exported, show_unexported);
+        if (show_unexported)
         {
             result.insert(result.end(), env_electric.begin(), env_electric.end());
         }
 
-        if (get_names_show_exported)
+        if (show_exported)
         {
             result.push_back(L"COLUMNS");
             result.push_back(L"LINES");
@@ -1432,8 +1388,8 @@ wcstring_list_t env_get_names(int flags)
 
         wcstring_list_t uni_list;
         env_universal_get_names2(uni_list,
-                                 get_names_show_exported,
-                                 get_names_show_unexported);
+                                 show_exported,
+                                 show_unexported);
         names.insert(uni_list.begin(), uni_list.end());
     }
 
@@ -1459,11 +1415,11 @@ static void get_exported(const env_node_t *n, std::map<wcstring, wcstring> &h)
     for (iter = n->env.begin(); iter != n->env.end(); ++iter)
     {
         const wcstring &key = iter->first;
-        var_entry_t *val_entry = iter->second;
-        if (val_entry->exportv && (val_entry->val != ENV_NULL))
+        const var_entry_t &val_entry = iter->second;
+        if (val_entry.exportv && (val_entry.val != ENV_NULL))
         {
             // Don't use std::map::insert here, since we need to overwrite existing values from previous scopes
-            h[key] = val_entry->val;
+            h[key] = val_entry.val;
         }
     }
 }