23488: tidy up module interface and documentation

11 files changed, 257 insertions, 113 deletions

diff --git a/ChangeLog b/ChangeLog
index be2afb06a..1c00076d4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2007-05-29  Peter Stephenson  <pws@csr.com>
 
+	* 23488: README, Etc/zsh-development-guide, Src/builtin.c,
+	Src/init.c, Src/module.c, Src/module/zftp.c, Src/Zle/complete.c,
+	Src/Zle/zle_main.c, Src/Zle/zle_thingy.c, Src/Zle/zle_tricky.c:
+	tidy up module interface and documentation.
+
 	* 23486: Test/A01grammar.ztst, Test/C03traps.ztst,
 	Test/D07multibyte.ztst, Test/E01options.ztst, Test/ztst.zsh:
 	Use {fd} syntax to open fd's for tests that won't clash
diff --git a/Etc/zsh-development-guide b/Etc/zsh-development-guide
index 6e29ed842..cb645d416 100644
--- a/Etc/zsh-development-guide
+++ b/Etc/zsh-development-guide
@@ -229,7 +229,8 @@ the module's feature strctures for all standard features; space
 is left for abstract features at the end of the array and the names
 must be added by the module.  Note that heap memory should
 be used for this (zhalloc, etc.) as memory for the features array is not
-freed.
+freed; note also the pointers for the abstract features are not
+initialised so setting them is mandatory any time there are any present.
 
 A structure "struct features" should
 be used to contain all standard features as well as the number of
@@ -242,12 +243,14 @@ should be set to an array of the same length as *featuresp without the
 NULL, containing a 1 for every feature that is enabled and a zero for other
 feature.  By default features are disabled.  If *enablesp is not NULL, its
 values should be used to decide whether features are to be turned off.  It
-should return status 0 for success, 1 on a failure to alter a feature.
-The function handlefeatures() conveniently handles all standard features
-present in the module's features structure; abstract features must
-be handled by the module.  As with features_, any handling of the
-array by the module itself should take into account that the array
-will not be freed and any allocation should therefore be from heap memory.
+should return status 0 for success, 1 on a failure to alter a feature.  The
+function handlefeatures() conveniently handles all standard features
+present in the module's features structure; abstract features must be
+handled by the module (as with the features array, the area of the enables
+array for abstract features is not even initialised by the main shell).  As
+with features_, any handling of the array by the module itself should take
+into account that the array will not be freed and any allocation should
+therefore be from heap memory.
 
 The functions features_ and enables_ can be called at any point
 after setup_ has been called and before cleanup_ is called.  In
diff --git a/README b/README
index 3c4254b16..034f5f39a 100644
--- a/README
+++ b/README
@@ -137,6 +137,11 @@ set non-ASCII characters for history or comments.  Multibyte characters
 have never worked and the most consistent change was to restrict the
 set to portable characters only.
 
+Writers of add-on modules should note that the API has changed
+significantly to allow user control of individual features provided by
+modules.  See the documentation for zmodload -F and
+Etc/zsh-development-guide, in that order.
+
 Documentation
 -------------
 
diff --git a/Src/Modules/zftp.c b/Src/Modules/zftp.c
index 89b869592..5c8822cc5 100644
--- a/Src/Modules/zftp.c
+++ b/Src/Modules/zftp.c
@@ -3174,7 +3174,7 @@ static struct features module_features = {
 int
 setup_(UNUSED(Module m))
 {
-    return (require_module("", "zsh/net/tcp", NULL) == 1);
+    return (require_module("zsh/net/tcp", NULL) == 1);
 }
 
 /**/
diff --git a/Src/Zle/complete.c b/Src/Zle/complete.c
index 7d8087052..245f73126 100644
--- a/Src/Zle/complete.c
+++ b/Src/Zle/complete.c
@@ -1524,7 +1524,7 @@ boot_(Module m)
     addhookfunc("reverse_menu", (Hookfn) reverse_menu);
     addhookfunc("list_matches", (Hookfn) list_matches);
     addhookfunc("invalidate_list", (Hookfn) invalidate_list);
-    addhookdefs(m->nam, comphooks, sizeof(comphooks)/sizeof(*comphooks));
+    (void)addhookdefs(m->nam, comphooks, sizeof(comphooks)/sizeof(*comphooks));
     return addwrapper(m, wrapper);
 }
 
@@ -1539,7 +1539,8 @@ cleanup_(Module m)
     deletehookfunc("reverse_menu", (Hookfn) reverse_menu);
     deletehookfunc("list_matches", (Hookfn) list_matches);
     deletehookfunc("invalidate_list", (Hookfn) invalidate_list);
-    deletehookdefs(m->nam, comphooks, sizeof(comphooks)/sizeof(*comphooks));
+    (void)deletehookdefs(m->nam, comphooks,
+			 sizeof(comphooks)/sizeof(*comphooks));
     deletewrapper(m, wrapper);
     return setfeatureenables(m->nam, &module_features, NULL);
 }
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 8ab10e5de..d2a665188 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1844,7 +1844,7 @@ boot_(Module m)
 {
     addhookfunc("before_trap", (Hookfn) zlebeforetrap);
     addhookfunc("after_trap", (Hookfn) zleaftertrap);
-    addhookdefs(m->nam, zlehooks, sizeof(zlehooks)/sizeof(*zlehooks));
+    (void)addhookdefs(m->nam, zlehooks, sizeof(zlehooks)/sizeof(*zlehooks));
     return 0;
 }
 
@@ -1858,7 +1858,7 @@ cleanup_(Module m)
     }
     deletehookfunc("before_trap", (Hookfn) zlebeforetrap);
     deletehookfunc("after_trap", (Hookfn) zleaftertrap);
-    deletehookdefs(m->nam, zlehooks, sizeof(zlehooks)/sizeof(*zlehooks));
+    (void)deletehookdefs(m->nam, zlehooks, sizeof(zlehooks)/sizeof(*zlehooks));
     return setfeatureenables(m->nam, &module_features, NULL);
 }
 
diff --git a/Src/Zle/zle_thingy.c b/Src/Zle/zle_thingy.c
index 42e32f142..536ddcd6a 100644
--- a/Src/Zle/zle_thingy.c
+++ b/Src/Zle/zle_thingy.c
@@ -590,7 +590,7 @@ bin_zle_complete(char *name, char **args, UNUSED(Options ops), UNUSED(char func)
     Thingy t;
     Widget w, cw;
 
-    if (require_module(name, "zsh/complete", NULL) == 1) {
+    if (require_module("zsh/complete", NULL) == 1) {
 	zwarnnam(name, "can't load complete module");
 	return 1;
     }
diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index 2b8e17c22..602d23ceb 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -612,7 +612,7 @@ docomplete(int lst)
      * no completion widgets are defined. */
 
     if (!module_loaded("zsh/compctl") && !hascompwidgets)
-	(void)load_module("zsh/compctl", NULL);
+	(void)load_module("zsh/compctl", NULL, 0);
 
     if (runhookdef(BEFORECOMPLETEHOOK, (void *) &lst)) {
 	active = 0;
diff --git a/Src/builtin.c b/Src/builtin.c
index 17af59398..20bc37c21 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -163,7 +163,7 @@ createbuiltintable(void)
     builtintab->freenode    = freebuiltinnode;
     builtintab->printnode   = printbuiltinnode;
 
-    addbuiltins("zsh", builtins, sizeof(builtins)/sizeof(*builtins));
+    (void)addbuiltins("zsh", builtins, sizeof(builtins)/sizeof(*builtins));
 }
 
 /* Print a builtin */
diff --git a/Src/init.c b/Src/init.c
index fce8adcd5..13078ea2e 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -701,7 +701,7 @@ setupvals(void)
 	    close(tmppipe[1]);
     }
 
-    addhookdefs(argzero, zshhooks, sizeof(zshhooks)/sizeof(*zshhooks));
+    (void)addhookdefs(argzero, zshhooks, sizeof(zshhooks)/sizeof(*zshhooks));
 
     init_eprog();
 
@@ -979,7 +979,7 @@ run_init_scripts(void)
 		 * Always attempt to load the newuser module to perform
 		 * checks for new zsh users.  Don't care if we can't load it.
 		 */
-		if (!load_module_silence("zsh/newuser", NULL, 1)) {
+		if (!load_module("zsh/newuser", NULL, 1)) {
 		    /* Unload it immediately. */
 		    unload_named_module("zsh/newuser", "zsh", 1);
 		}
@@ -1152,7 +1152,7 @@ init_bltinmods(void)
 
 #include "bltinmods.list"
 
-    (void)load_module("zsh/main", NULL);
+    (void)load_module("zsh/main", NULL, 0);
 }
 
 /**/
@@ -1213,8 +1213,8 @@ char *
 autoload_zleread(char **lp, char **rp, int ha, int con)
 {
     zlereadptr = fallback_zleread;
-    if (load_module("zsh/zle", NULL) != 1)
-	(void)load_module("zsh/compctl", NULL);
+    if (load_module("zsh/zle", NULL, 0) != 1)
+	(void)load_module("zsh/compctl", NULL, 0);
     return zlereadptr(lp, rp, ha, con);
 }
 
@@ -1240,7 +1240,7 @@ static void
 autoload_zlesetkeymap(int mode)
 {
     zlesetkeymapptr = noop_function_int;
-    (void)load_module("zsh/zle", NULL);
+    (void)load_module("zsh/zle", NULL, 0);
     (*zlesetkeymapptr)(mode);
 }
 
diff --git a/Src/module.c b/Src/module.c
index a6329850e..c837193e8 100644
--- a/Src/module.c
+++ b/Src/module.c
@@ -221,11 +221,31 @@ deletebuiltin(char *nam)
     return 0;
 }
 
+/*
+ * Manipulate a set of builtins.  This should be called
+ * via setfeatureenables() (or, usually, via the next level up,
+ * handlefeatures()).
+ *
+ * "nam" is the name of the calling code builtin, probably "zmodload".
+ *
+ * "binl" is the builtin table containing an array of "size" builtins.
+ *
+ * "e" is either NULL, in which case all builtins in the
+ * table are removed, or else an array corresponding to "binl"
+ * with a 1 for builtins that are to be added and a 0 for builtins
+ * that are to be removed.  Any builtin already in the appropriate
+ * state is left alone.
+ *
+ * Returns 1 on any error, 0 for success.  The recommended way
+ * of handling errors is to compare the enables passed down
+ * with the set retrieved after the error to find what failed.
+ */
+
 /**/
-mod_export int
+static int
 setbuiltins(char const *nam, Builtin binl, int size, int *e)
 {
-    int hads = 0, hadf = 0, n;
+    int ret = 0, n;
 
     for(n = 0; n < size; n++) {
 	Builtin b = &binl[n];
@@ -235,24 +255,22 @@ setbuiltins(char const *nam, Builtin binl, int size, int *e)
 	    if (addbuiltin(b)) {
 		zwarnnam(nam,
 			 "name clash when adding builtin `%s'", b->node.nam);
-		hadf = 1;
+		ret = 1;
 	    } else {
 		b->node.flags |= BINF_ADDED;
-		hads = 2;
 	    }
 	} else {
 	    if (!(b->node.flags & BINF_ADDED))
 		continue;
 	    if (deletebuiltin(b->node.nam)) {
 		zwarnnam(nam, "builtin `%s' already deleted", b->node.nam);
-		hadf = 1;
+		ret = 1;
 	    } else {
-		hads = 2;
 		b->node.flags &= ~BINF_ADDED;
 	    }
 	}
     }
-    return hadf ? hads : 1;
+    return ret;
 }
 
 /*
@@ -261,8 +279,7 @@ setbuiltins(char const *nam, Builtin binl, int size, int *e)
  * added; that flag is set if they succeed.
  *
  * If any fail, an error message is printed, using nam as the leading name.
- * Returns 1 if all additions succeed, 2 if some succeed and some fail, and 0
- * if all (and at least 1) fail.
+ * Returns 0 on success, 1 for any failure.
  *
  * This should not be used from a module; instead, use handlefeatures().
  */
@@ -271,7 +288,7 @@ setbuiltins(char const *nam, Builtin binl, int size, int *e)
 mod_export int
 addbuiltins(char const *nam, Builtin binl, int size)
 {
-    int hads = 0, hadf = 0, n;
+    int ret = 0, n;
 
     for(n = 0; n < size; n++) {
 	Builtin b = &binl[n];
@@ -279,13 +296,12 @@ addbuiltins(char const *nam, Builtin binl, int size)
 	    continue;
 	if(addbuiltin(b)) {
 	    zwarnnam(nam, "name clash when adding builtin `%s'", b->node.nam);
-	    hadf = 1;
+	    ret = 1;
 	} else {
 	    b->node.flags |= BINF_ADDED;
-	    hads = 2;
 	}
     }
-    return hadf ? hads : 1;
+    return ret;
 }
 
 
@@ -444,9 +460,9 @@ deleteconddef(Conddef c)
     if (p) {
 	if (q)
 	    q->next = p->next;
-	else 
+	else
 	    condtab = p->next;
-		
+
 	if (p->module) {
 	    /* autoloaded, free it */
 	    zsfree(p->name);
@@ -458,11 +474,16 @@ deleteconddef(Conddef c)
     return -1;
 }
 
+/*
+ * Add or remove sets of conditions.  The interface is
+ * identical to setbuiltins().
+ */
+
 /**/
-mod_export int
+static int
 setconddefs(char const *nam, Conddef c, int size, int *e)
 {
-    int hads = 0, hadf = 0;
+    int ret = 0;
 
     while (size--) {
 	if (e && *e++) {
@@ -473,10 +494,9 @@ setconddefs(char const *nam, Conddef c, int size, int *e)
 	    if (addconddef(c)) {
 		zwarnnam(nam, "name clash when adding condition `%s'",
 			 c->name);
-		hadf = 1;
+		ret = 1;
 	    } else {
 		c->flags |= CONDF_ADDED;
-		hads = 2;
 	    }
 	} else {
 	    if (!(c->flags & CONDF_ADDED)) {
@@ -485,15 +505,14 @@ setconddefs(char const *nam, Conddef c, int size, int *e)
 	    }
 	    if (deleteconddef(c)) {
 		zwarnnam(nam, "condition `%s' already deleted", c->name);
-		hadf = 1;
+		ret = 1;
 	    } else {
 		c->flags &= ~CONDF_ADDED;
-		hads = 2;
 	    }
 	}
 	c++;
     }
-    return hadf ? hads : 1;
+    return ret;
 }
 
 /* This adds a definition for autoloading a module for a condition. */
@@ -565,17 +584,16 @@ addhookdef(Hookdef h)
 mod_export int
 addhookdefs(char const *nam, Hookdef h, int size)
 {
-    int hads = 0, hadf = 0;
+    int ret = 0;
 
     while (size--) {
 	if (addhookdef(h)) {
 	    zwarnnam(nam, "name clash when adding hook `%s'", h->name);
-	    hadf = 1;
-	} else
-	    hads = 2;
+	    ret = 1;
+	}
 	h++;
     }
-    return hadf ? hads : 1;
+    return ret;
 }
 
 /* Delete hook definitions. */
@@ -599,15 +617,20 @@ deletehookdef(Hookdef h)
     return 0;
 }
 
+/* Remove multiple hook definitions. */
+
 /**/
 mod_export int
 deletehookdefs(UNUSED(char const *nam), Hookdef h, int size)
 {
+    int ret = 0;
+
     while (size--) {
-	deletehookdef(h);
+	if (deletehookdef(h))
+	    ret = 1;
 	h++;
     }
-    return 1;
+    return ret;
 }
 
 /* Add a function to a hook. */
@@ -648,6 +671,8 @@ deletehookdeffunc(Hookdef h, Hookfn f)
     return 1;
 }
 
+/* Delete a hook. */
+
 /**/
 mod_export int
 deletehookfunc(char *n, Hookfn f)
@@ -683,22 +708,18 @@ runhookdef(Hookdef h, void *d)
 	return ((Hookfn) getdata(lastnode(h->funcs)))(h, d);
 }
 
-/**/
-int
-runhook(char *n, void *d)
-{
-    Hookdef h = gethookdef(n);
-
-    if (h)
-	return runhookdef(h, d);
-    return 0;
-}
 
 
 /************************************************************************
  * Shell parameters.
  ************************************************************************/
 
+/*
+ * Check that it's possible to add a parameter.  This
+ * requires that either there's no parameter already present,
+ * or it's a global parameter marked for autoloading.
+ */
+
 static int
 checkaddparam(char *nam)
 {
@@ -815,11 +836,16 @@ deleteparamdef(Paramdef d)
     return 0;
 }
 
+/*
+ * Add or remove sets of parameters.  The interface is
+ * identical to setbuiltins().
+ */
+
 /**/
-mod_export int
+static int
 setparamdefs(char const *nam, Paramdef d, int size, int *e)
 {
-    int hads = 0, hadf = 0;
+    int ret = 0;
 
     while (size--) {
 	if (e && *e++) {
@@ -829,9 +855,7 @@ setparamdefs(char const *nam, Paramdef d, int size, int *e)
 	    }
 	    if (addparamdef(d)) {
 		zwarnnam(nam, "error when adding parameter `%s'", d->name);
-		hadf = 1;
-	    } else {
-		hads = 2;
+		ret = 1;
 	    }
 	} else {
 	    if (!d->pm) {
@@ -840,14 +864,12 @@ setparamdefs(char const *nam, Paramdef d, int size, int *e)
 	    }
 	    if (deleteparamdef(d)) {
 		zwarnnam(nam, "parameter `%s' already deleted", d->name);
-		hadf = 1;
-	    } else {
-		hads = 2;
+		ret = 1;
 	    }
 	}
 	d++;
     }
-    return hadf ? hads : 1;
+    return ret;
 }
 
 /* This adds a definition for autoloading a module for a parameter. */
@@ -880,6 +902,11 @@ add_autoparam(char *nam, char *module)
 /**/
 MathFunc mathfuncs;
 
+/*
+ * Remove a single math function form the list (utility function).
+ * This does not delete a module math function, that's deletemathfunc().
+ */
+
 /**/
 void
 removemathfunc(MathFunc previous, MathFunc current)
@@ -894,6 +921,8 @@ removemathfunc(MathFunc previous, MathFunc current)
     zfree(current, sizeof(*current));
 }
 
+/* Find a math function in the list, handling autoload if necessary. */
+
 /**/
 MathFunc
 getmathfunc(char *name, int autol)
@@ -917,6 +946,8 @@ getmathfunc(char *name, int autol)
     return NULL;
 }
 
+/* Add a single math function */
+
 /**/
 static int
 addmathfunc(MathFunc f)
@@ -944,6 +975,8 @@ addmathfunc(MathFunc f)
     return 0;
 }
 
+/* Delete a single math function */
+
 /**/
 mod_export int
 deletemathfunc(MathFunc f)
@@ -971,11 +1004,16 @@ deletemathfunc(MathFunc f)
     return -1;
 }
 
+/*
+ * Add or remove sets of math functions.  The interface is
+ * identical to setbuiltins().
+ */
+
 /**/
-mod_export int
+static int
 setmathfuncs(char const *nam, MathFunc f, int size, int *e)
 {
-    int hads = 0, hadf = 0;
+    int ret = 0;
 
     while (size--) {
 	if (e && *e++) {
@@ -986,10 +1024,9 @@ setmathfuncs(char const *nam, MathFunc f, int size, int *e)
 	    if (addmathfunc(f)) {
 		zwarnnam(nam, "name clash when adding math function `%s'",
 			 f->name);
-		hadf = 1;
+		ret = 1;
 	    } else {
 		f->flags |= MFF_ADDED;
-		hads = 2;
 	    }
 	} else {
 	    if (!(f->flags & MFF_ADDED)) {
@@ -998,17 +1035,18 @@ setmathfuncs(char const *nam, MathFunc f, int size, int *e)
 	    }
 	    if (deletemathfunc(f)) {
 		zwarnnam(nam, "math function `%s' already deleted", f->name);
-		hadf = 1;
+		ret = 1;
 	    } else {
 		f->flags &= ~MFF_ADDED;
-		hads = 2;
-	    }	    
+	    }
 	}
 	f++;
     }
-    return hadf ? hads : 1;
+    return ret;
 }
 
+/* Add an autoload definition for a math function. */
+
 /**/
 int
 add_automathfunc(char *nam, char *module)
@@ -1147,6 +1185,12 @@ hpux_dlsym(void *handle, char *name)
 # define RTLD_GLOBAL 0
 #endif
 
+/*
+ * Attempt to load a module.  This is the lowest level of
+ * zsh function for dynamical modules.  Returns the handle
+ * from the dynamic loader.
+ */
+
 /**/
 static void *
 try_load_module(char const *name)
@@ -1167,6 +1211,11 @@ try_load_module(char const *name)
     return ret;
 }
 
+/*
+ * Load a module, with option to complain or not.
+ * Returns the handle from the dynamic loader.
+ */
+
 /**/
 static void *
 do_load_module(char const *name, int silent)
@@ -1182,6 +1231,10 @@ do_load_module(char const *name, int silent)
 /**/
 #else /* !DYNAMIC */
 
+/*
+ * Dummy loader when no dynamic loading available; always fails.
+ */
+
 /**/
 static void *
 do_load_module(char const *name, int silent)
@@ -1249,6 +1302,13 @@ delete_module(LinkNode node)
     zfree(m, sizeof(*m));
 }
 
+/*
+ * Return 1 if a module is fully loaded else zero.
+ * A linked module may be marked as unloaded even though
+ * we can't fully unload it; this returns 0 to try to
+ * make that state transparently like an unloaded module.
+ */
+
 /**/
 mod_export int
 module_loaded(const char *name)
@@ -1650,6 +1710,11 @@ do_cleanup_module(Module m)
 	(m->u.handle && cleanup_module(m));
 }
 
+/*
+ * Test a module name contains only valid characters: those
+ * allowed in a shell identifier plus slash.  Return 1 if so.
+ */
+
 /**/
 static int
 modname_ok(char const *p)
@@ -1663,25 +1728,24 @@ modname_ok(char const *p)
 }
 
 /*
+ * High level function to load a module, encapsulating
+ * all the handling of module functions.
+ *
+ * "*enablesstr" is NULL if the caller is not feature-aware;
+ * then the module should turn on all features.  If it
+ * is not NULL it points to an array of features to be
+ * turned on.  This function is responsible for testing whether
+ * the module supports those features.
+ *
+ * If "silent" is 1, don't issue warnings for errors.
+ *
  * Now returns 0 for success (changed post-4.3.4),
  * 1 for complete failure, 2 if some features couldn't be set.
  */
 
 /**/
 mod_export int
-load_module(char const *name, char **enablesstr)
-{
-    return load_module_silence(name, enablesstr, 0);
-}
-
-/*
- * Returns 0 for success (changed post-4.3.4), 1 for complete
- * failure, 2 if some features couldn't be set.
- */
-
-/**/
-mod_export int
-load_module_silence(char const *name, char **enablesstr, int silent)
+load_module(char const *name, char **enablesstr, int silent)
 {
     Module m;
     void *handle = NULL;
@@ -1754,7 +1818,7 @@ load_module_silence(char const *name, char **enablesstr, int silent)
      */
     if (m->deps)
 	for (n = firstnode(m->deps); n; incnode(n))
-	    if (load_module_silence((char *) getdata(n), NULL, silent) == 1) {
+	    if (load_module((char *) getdata(n), NULL, silent) == 1) {
 		m->flags &= ~MOD_BUSY;
 		unqueue_signals();
 		return 1;
@@ -1803,9 +1867,9 @@ load_module_silence(char const *name, char **enablesstr, int silent)
     return bootret;
 }
 
-/* This ensures that the module with the name given as the second argument
+/* This ensures that the module with the name given as the first argument
  * is loaded.
- * The last argument is the array of features to set.  If this is NULL
+ * The other argument is the array of features to set.  If this is NULL
  * and the module needs to be loaded, all features are enabled.
  * If this is non-NULL the module features are set accordingly
  * whether or not the module is loaded; it is an error if the
@@ -1815,11 +1879,15 @@ load_module_silence(char const *name, char **enablesstr, int silent)
  * The return value is 0 if the module was found or loaded
  * (this changed post-4.3.4, because I got so confused---pws),
  * 1 if loading failed completely, 2 if some features couldn't be set.
+ *
+ * This function behaves like load_module() except that it
+ * handles the case where the module was already loaded, and
+ * sets features accordingly.
  */
 
 /**/
 mod_export int
-require_module(char *nam, const char *module, char **features)
+require_module(const char *module, char **features)
 {
     Module m = NULL;
     LinkNode node;
@@ -1830,7 +1898,7 @@ require_module(char *nam, const char *module, char **features)
     node = find_module(module, 1, &module);
     if (!node || !(m = ((Module) getdata(node)))->u.handle ||
 	(m->flags & MOD_UNLOAD))
-	ret = load_module_silence(module, features, 0);
+	ret = load_module(module, features, 0);
     else
 	ret = do_module_features(m, features, 0);
     unqueue_signals();
@@ -1838,6 +1906,11 @@ require_module(char *nam, const char *module, char **features)
     return ret;
 }
 
+/*
+ * Indicate that the module named "name" depends on the module
+ * named "from".
+ */
+
 /**/
 void
 add_dep(const char *name, char *from)
@@ -1872,6 +1945,11 @@ add_dep(const char *name, char *from)
 	zaddlinknode(m->deps, ztrdup(from));
 }
 
+/*
+ * Function to be used when scanning the builtins table to
+ * find and print autoloadable builtins.
+ */
+
 /**/
 static void
 autoloadscan(HashNode hn, int printflags)
@@ -1905,6 +1983,10 @@ autoloadscan(HashNode hn, int printflags)
  * Handling for the zmodload builtin and its various options.
  ************************************************************************/
 
+/*
+ * Main builtin entry point for zmodload.
+ */
+
 /**/
 int
 bin_zmodload(char *nam, char **args, Options ops, UNUSED(int func))
@@ -1982,6 +2064,8 @@ bin_zmodload(char *nam, char **args, Options ops, UNUSED(int func))
     return ret;
 }
 
+/* zmodload -A */
+
 /**/
 static int
 bin_zmodload_alias(char *nam, char **args, Options ops)
@@ -2093,6 +2177,8 @@ bin_zmodload_alias(char *nam, char **args, Options ops)
     return 0;
 }
 
+/* zmodload -e (without -F) */
+
 /**/
 static int
 bin_zmodload_exist(UNUSED(char *nam), char **args, Options ops)
@@ -2112,7 +2198,7 @@ bin_zmodload_exist(UNUSED(char *nam), char **args, Options ops)
 		    m = (Module) getdata(node2);
 		else
 		    continue;
-	    } 
+	    }
 	    if (m->u.handle && !(m->flags & MOD_UNLOAD)) {
 		nicezputs(modname, stdout);
 		putchar('\n');
@@ -2132,6 +2218,8 @@ bin_zmodload_exist(UNUSED(char *nam), char **args, Options ops)
     }
 }
 
+/* zmodload -d */
+
 /**/
 static int
 bin_zmodload_dep(UNUSED(char *nam), char **args, Options ops)
@@ -2205,6 +2293,8 @@ bin_zmodload_dep(UNUSED(char *nam), char **args, Options ops)
     }
 }
 
+/* zmodload -ab */
+
 /**/
 static int
 bin_zmodload_auto(char *nam, char **args, Options ops)
@@ -2249,6 +2339,8 @@ bin_zmodload_auto(char *nam, char **args, Options ops)
     }
 }
 
+/* zmodload -ac */
+
 /**/
 static int
 bin_zmodload_cond(char *nam, char **args, Options ops)
@@ -2313,6 +2405,8 @@ bin_zmodload_cond(char *nam, char **args, Options ops)
     }
 }
 
+/* zmodload -af */
+
 /**/
 static int
 bin_zmodload_math(char *nam, char **args, Options ops)
@@ -2369,6 +2463,11 @@ bin_zmodload_math(char *nam, char **args, Options ops)
     }
 }
 
+/*
+ * Function for scanning the parameter table to find and print
+ * out autoloadable parameters.
+ */
+
 static void
 printautoparams(HashNode hn, int lon)
 {
@@ -2382,6 +2481,8 @@ printautoparams(HashNode hn, int lon)
     }
 }
 
+/* zmodload -ap */
+
 /**/
 static int
 bin_zmodload_param(char *nam, char **args, Options ops)
@@ -2425,6 +2526,8 @@ bin_zmodload_param(char *nam, char **args, Options ops)
     }
 }
 
+/* Backend handler for zmodload -u */
+
 /**/
 int
 unload_module(Module m, LinkNode node)
@@ -2513,6 +2616,11 @@ unload_module(Module m, LinkNode node)
     return 0;
 }
 
+/*
+ * Unload a module by name (modname); nam is the command name.
+ * Optionally don't print some error messages (always print
+ * dependency errors).
+ */
 
 /**/
 int
@@ -2556,6 +2664,7 @@ unload_named_module(char *modname, char *nam, int silent)
     return ret;
 }
 
+/* zmodload -u without -d */
 
 /**/
 static int
@@ -2590,7 +2699,7 @@ bin_zmodload_load(char *nam, char **args, Options ops)
     } else {
 	/* load modules */
 	for (; *args; args++) {
-	    int tmpret = require_module(nam, *args, NULL);
+	    int tmpret = require_module(*args, NULL);
 	    if (tmpret && ret != 1)
 		ret = tmpret;
 	}
@@ -2599,6 +2708,8 @@ bin_zmodload_load(char *nam, char **args, Options ops)
     }
 }
 
+/* zmodload -F */
+
 /**/
 static int
 bin_zmodload_features(char *nam, char **args, Options ops)
@@ -2743,7 +2854,7 @@ bin_zmodload_features(char *nam, char **args, Options ops)
 	return 1;
     }
 
-    return require_module(nam, modname, args);
+    return require_module(modname, args);
 }
 
 
@@ -2834,23 +2945,36 @@ setfeatureenables(char const *nam, Features f, int *e)
 {
     int ret = 0;
 
-    if (f->bn_size && setbuiltins(nam, f->bn_list, f->bn_size, e) != 1)
-	ret = 1;
-    if (e)
-	e += f->bn_size;
-    if (f->cd_size && setconddefs(nam, f->cd_list, f->cd_size, e) != 1)
-	ret = 1;
-    if (e)
-	e += f->cd_size;
-    if (f->pd_size && setparamdefs(nam, f->pd_list, f->pd_size, e) != 1)
-	ret = 1;
-    if (e)
-	e += f->pd_size;
-    if (f->mf_size && setmathfuncs(nam, f->mf_list, f->mf_size, e) != 1)
-	ret = 1;
+    if (f->bn_size) {
+	if (setbuiltins(nam, f->bn_list, f->bn_size, e))
+	    ret = 1;
+	if (e)
+	    e += f->bn_size;
+    }
+    if (f->cd_size) {
+	if (setconddefs(nam, f->cd_list, f->cd_size, e))
+	    ret = 1;
+	if (e)
+	    e += f->cd_size;
+    }
+    if (f->pd_size) {
+	if (setparamdefs(nam, f->pd_list, f->pd_size, e))
+	    ret = 1;
+	if (e)
+	    e += f->pd_size;
+    }
+    if (f->mf_size) {
+	if (setmathfuncs(nam, f->mf_list, f->mf_size, e))
+	    ret = 1;
+    }
     return ret;
 }
 
+/*
+ * Convenient front-end to get or set features which
+ * can be used in a module enables_() function.
+ */
+
 /**/
 mod_export int
 handlefeatures(char *nam, Features f, int **enables)
@@ -2864,6 +2988,12 @@ handlefeatures(char *nam, Features f, int **enables)
 /*
  * Ensure module "modname" is providing feature with "prefix"
  * and "feature" (e.g. "b:", "limit").
+ *
+ * This will usually be called from the main shell to handle
+ * loading of an autoloadable feature.
+ *
+ * Returns 0 on success, 1 for error in module, 2 for error
+ * setting the feature.
  */
 
 /**/
@@ -2875,5 +3005,5 @@ ensurefeature(char *modname, char *prefix, char *feature)
 
     features[0] = f;
     features[1] = NULL;
-    return require_module(NULL, modname, features);
+    return require_module(modname, features);
 }