Re: [PATCH] Refactor resolve_nameref() and define getparam(), loadparama(), and resolveparam()

[Philippe Altherr <philippe.altherr@xxxxxxxxx>]

loadparam() may be clearly defined, but at the end of your patch
series, it has no callers in the codebase. Remove? loadparam_pm only
has one caller (excluding the unused one in loadparam), maybe it
should be a static helper instead, since the caller is static? That
absolves you from wanting to keep the unused string name wrapper for
symmetry with get/resolve/resolveref.

Interesting, I thought that there would be calls to the loadparam functions. In hindsight, it actually makes some sense; if you want to force the loading of a parameter, you certainly also want to resolve it if it happens that the starting point is a reference. I agree that we can make it static. I would also drop the _pm suffix.

(i'm not sold on "asset" either btw).

I tried to eliminate asset_pm (attached patch). It turns out that it doesn't buy us much. Eliminating it seems to be the right way.

The changes in Zle/zle_tricky.c are motivated by the fact that keypm is described as being an assoc array and the only place that reads keypm assumes that it's an assoc array.

> Defines isset_pm() and asset_pm() to test/ensure whether/that a parameter is a set one.

You have getparam/getparam_pm, loadparam/loadparam_pm and
resolveparam(ref)/resolveparam(ref)_pm where the non-_pm are wrappers
that call getparam() on the name for the caller. But then you have
isset_pm and asset_pm with no corresponding isset or asset, and in
fact, isset() is an existing macro that checks if an option is set.
Maybe call these latter two param_isset and param_asset instead, since
the _pm doesn't denote a difference from a version that takes a string
name argument (i'm not sold on "asset" either btw).

I agree that this is an abuse of the _pm suffix trick. What about isparamset? I think that it matches better how functions are named in params.c than param_isset (and we don't need an asset version).

- /* Check for self references */
- if (refname && *refname && basepm != pm) {
-    dont_queue_signals(); /* Prevent unkillable loops */
-    basepm = resolve_nameref_rec(pm, pm, 0);
-    restore_queue_signals(q);
- }

Do we have some proof that these unkillable loops can not happen
anymore? I couldn't find some motivation for removing this.

The code is now such that references are checked for loops whenever a reference is created or modified and any detected loop is immediately undone by unsetting the reference that created the last link in the loop. Thus, the parameter table never contains permanent loops. Whenever a reference is created or modified, it either creates a loop that refers back to itself or it doesn't create a loop. It can never refer to an already existing loop. Thus, detecting a loop can always be done by checking that a newly created or modified reference doesn't refer back to itself.

Let me know if you agree with the proposed code/name changes and I will update the patches.

Philippe

diff --git a/Src/Modules/db_gdbm.c b/Src/Modules/db_gdbm.c
index 0cd9a23d4..4628f67fd 100644
--- a/Src/Modules/db_gdbm.c
+++ b/Src/Modules/db_gdbm.c
@@ -130,7 +130,7 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
     resource_name = OPT_ARG(ops, 'f');
     pmname = *args;
 
-    if ((tied_param = asset_pm(resolveparam(pmname, 0)))) {
+    if ((tied_param = resolveparam(pmname, 0)) && isset_pm(tied_param)) {
 	/*
 	 * Unset any existing parameter.  Note there's no implicit
 	 * "local" here, but if the existing parameter is local
diff --git a/Src/Modules/parameter.c b/Src/Modules/parameter.c
index f6c4c074f..e92d1211a 100644
--- a/Src/Modules/parameter.c
+++ b/Src/Modules/parameter.c
@@ -93,7 +93,7 @@ paramtypestr(Param pm, int append)
 
     if (append) {
 	if ((pm->node.flags & PM_NAMEREF) &&
-	    (pm = asset_pm(resolveparam_pm(pm, 0)))) {
+	    (pm = resolveparam_pm(pm, 0)) && isset_pm(pm)) {
 	    val = zhtricat(val, "-", paramtypestr(pm, 0));
 	}
     }
@@ -104,13 +104,12 @@ paramtypestr(Param pm, int append)
 static HashNode
 getpmparameter(UNUSED(HashTable ht), const char *name)
 {
-    Param rpm, pm = NULL;
+    Param rpm = getparam(name), pm = (Param) hcalloc(sizeof(struct param));
 
-    pm = (Param) hcalloc(sizeof(struct param));
     pm->node.nam = dupstring(name);
     pm->node.flags = PM_SCALAR | PM_READONLY;
     pm->gsu.s = &nullsetscalar_gsu;
-    if ((rpm = asset_pm(getparam(name)))) {
+    if (isset_pm(rpm)) {
 	pm->u.str = paramtypestr(rpm, 1);
     } else {
 	pm->u.str = dupstring("");
diff --git a/Src/Modules/zftp.c b/Src/Modules/zftp.c
index cc32b29f8..a954c5ce0 100644
--- a/Src/Modules/zftp.c
+++ b/Src/Modules/zftp.c
@@ -490,10 +490,10 @@ zfmovefd(int fd)
 static void
 zfsetparam(char *name, void *val, int flags)
 {
-    Param pm = NULL;
+    Param pm = getparam(name);
     int type = (flags & ZFPM_INTEGER) ? PM_INTEGER : PM_SCALAR;
 
-    if (!(pm = asset_pm(getparam(name)))) {
+    if (!pm || !isset_pm(pm)) {
 	/*
 	 * just make it readonly when creating, in case user
 	 * *really* knows what they're doing
@@ -524,9 +524,9 @@ zfsetparam(char *name, void *val, int flags)
 static void
 zfunsetparam(char *name)
 {
-    Param pm;
+    Param pm = getparam(name);
 
-    if ((pm = asset_pm(getparam(name)))) {
+    if (isset_pm(pm)) {
 	pm->node.flags &= ~PM_READONLY;
 	unsetparam_pm(pm, 0, 1);
     }
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 8aca7a0cb..b39fdf43c 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1867,8 +1867,8 @@ bin_vared(char *name, char **args, Options ops, UNUSED(int func))
 	createparam(args[0], type);
     }
     queue_signals();
-    pm = asset_pm(resolveparam(args[0], 1));
-    if (pm && (PM_TYPE(pm->node.flags) & (PM_ARRAY|PM_HASHED))) {
+    pm = resolveparam(args[0], 1);
+    if (isset_pm(pm) && (PM_TYPE(pm->node.flags) & (PM_ARRAY|PM_HASHED))) {
 	char **a;
 
 	/*
diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index c0f5d2a2b..1085f0643 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -1512,11 +1512,13 @@ get_comp_string(void)
 	    zlemetacs_qsub - wb) {
 	    s = NULL;
 	    inwhat = IN_MATH;
-	    if ((keypm = asset_pm(resolveparam(varname, 1))) &&
+	    if ((keypm = resolveparam(varname, 1)) && isset_pm(keypm) &&
 		(keypm->node.flags & PM_HASHED))
 		insubscr = 2;
-	    else
+	    else {
+		keypm = NULL;
 		insubscr = 1;
+	    }
 	} else if (*s == '=') {
             if (zlemetacs_qsub > wb + (s - tt)) {
                 s++;
@@ -1612,9 +1614,11 @@ get_comp_string(void)
 		zsfree(varname);
 		varname = ztrdup(nb);
 		*ne = sav;
-		if ((keypm = asset_pm(resolveparam(varname, 1))) &&
+		if ((keypm = resolveparam(varname, 1)) && isset_pm(keypm) &&
 		    (keypm->node.flags & PM_HASHED))
 		    insubscr = 2;
+		else
+		    keypm = NULL;
 	    }
 	}
     }
@@ -1693,12 +1697,14 @@ get_comp_string(void)
 	    if (wptr < sqbr) {
 		zsfree(varname);
 		varname = ztrduppfx(wptr, sqbr - wptr);
-		if ((keypm = asset_pm(resolveparam(varname, 1))) &&
+		if ((keypm = resolveparam(varname, 1)) && isset_pm(keypm) &&
 		    (keypm->node.flags & PM_HASHED)) {
 		    if (insubscr != 3)
 			insubscr = 2;
-		} else
+		} else {
+		    keypm = NULL;
 		    insubscr = 1;
+		}
 	    }
 	}
 
diff --git a/Src/builtin.c b/Src/builtin.c
index 5bf8a49e1..ce59b7312 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -3786,7 +3786,6 @@ mkautofn(Shfunc shf)
 int
 bin_unset(char *name, char **argv, Options ops, int func)
 {
-    Param pm, next;
     Patprog pprog;
     char *s;
     int match = 0, returnval = 0;
@@ -3805,7 +3804,8 @@ bin_unset(char *name, char **argv, Options ops, int func)
 	    if ((pprog = patcompile(s, PAT_STATIC, NULL))) {
 		/* Go through the parameter table, and unset any matches */
 		for (i = 0; i < paramtab->hsize; i++) {
-		    for (pm = (Param) paramtab->nodes[i]; pm; pm = next) {
+		    for (Param pm = (Param) paramtab->nodes[i], next; pm;
+			 pm = next) {
 			/* record pointer to next, since we may free this one */
 			next = (Param) pm->node.next;
 			if (pattry(pprog, pm->node.nam)) {
@@ -3849,18 +3849,18 @@ bin_unset(char *name, char **argv, Options ops, int func)
 	    returnval = 1;
 	    continue;
 	}
-	pm = getparam(s);
 	if (!ss) {
-	    pm = asset_pm(OPT_ISSET(ops,'n') ? pm : resolveparam_pm(pm, 0));
+	    Param pm = OPT_ISSET(ops,'n') ? getparam(s) : resolveparam(s, 0);
 	    /*
 	     * Unsetting a nonexistent or unset variable is not an error.
 	     * This appears to be reasonably standard behaviour.
 	     */
-	    if (pm && unsetparam_pm(pm, 0, 1))
+	    if (isset_pm(pm) && unsetparam_pm(pm, 0, 1))
 		returnval = 1;
 	} else {
+	    Param pm = resolveparam(s, 1);
 	    *ss = '[';
-	    if (!(pm = asset_pm(resolveparam_pm(pm, 1)))) {
+	    if (!isset_pm(pm)) {
 		/* warning? */
 		continue;
 	    }
diff --git a/Src/params.c b/Src/params.c
index e0a17d5b7..67a6e3af9 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -579,15 +579,6 @@ isset_pm(Param pm)
 	(!(pm->node.flags & PM_UNSET) || (pm->node.flags & PM_DECLARED));
 }
 
-/* Returns the parameter if it is set, otherwise NULL. */
-
-/**/
-mod_export Param
-asset_pm(Param pm)
-{
-    return isset_pm(pm) ? pm : NULL;
-}
-
 /* Returns whether the parameter is a placeholder reference. */
 
 /**/
diff --git a/Src/utils.c b/Src/utils.c
index acb4317e7..da081efe3 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -1254,7 +1254,7 @@ getnameddir(char *name)
     /* Check if there is a scalar parameter with this name whose value *
      * begins with a `/'.  If there is, add it to the hash table and   *
      * return the new value.                                           */
-    if ((pm = asset_pm(getparam(name))) && !pm->level &&
+    if ((pm = getparam(name)) && isset_pm(pm) && !pm->level &&
 	    (PM_TYPE(pm->node.flags) == PM_SCALAR) &&
 	    (str = getsparam(name)) && *str == '/') {
 	pm->node.flags |= PM_NAMEDDIR;