Re: roff conditional recursion

[Kristaps Dzonsons <kristaps@bsd.lv>]

[-- Attachment #1: Type: text/plain, Size: 672 bytes --]

On 07/04/2011 02:39, Joerg Sonnenberger wrote:
> Hi Ingo,
> at the moment, as soon as ROFF_ie is seen, r->stackpos is incremented.
> This happens in roff.c 1028. But the corresponding code for handling
> ROFF_el in 995 doesn't decrement it again. Shouldn't it be
> 	rule = r->rstack[r->rstackpos--];
> in line 998?
>
> Attached is a trivial example.

Joerg,

Enclosed is a patch that follows this logic, plus some clean-up and 
documentation (it works on test.1 and doesn't seem to cause problems 
anywhere else).  The rstackpos, yes, can be safely "consumed" when the 
`el' is called, and need not wait until the nodes are popped.

Ingo, any comments?

Thanks,

Kristaps

[-- Attachment #2: patch.roff.txt --]
[-- Type: text/plain, Size: 2209 bytes --]

Index: roff.c
===================================================================
RCS file: /usr/vhosts/mdocml.bsd.lv/cvs/mdocml/roff.c,v
retrieving revision 1.131
diff -u -r1.131 roff.c
--- roff.c	5 Apr 2011 22:22:33 -0000	1.131
+++ roff.c	7 Apr 2011 09:57:11 -0000
@@ -277,10 +277,6 @@
 	assert(r->last);
 	p = r->last; 
 
-	if (ROFF_el == p->tok)
-		if (r->rstackpos > -1)
-			r->rstackpos--;
-
 	r->last = r->last->parent;
 	free(p->name);
 	free(p->end);
@@ -976,29 +972,20 @@
 	int		 sv;
 	enum roffrule	 rule;
 
-	/* Stack overflow! */
+	/* 
+	 * An `.el' has no conditional body: it will consume the value
+	 * of the current rstack entry set in prior `ie' calls or
+	 * defaults to DENY.  
+	 *
+	 * If we're not an `el', however, then evaluate the conditional.
+	 */
 
-	if (ROFF_ie == tok && r->rstackpos == RSTACK_MAX - 1) {
-		mandoc_msg(MANDOCERR_MEM, r->parse, ln, ppos, NULL);
-		return(ROFF_ERR);
-	}
-
-	/* First, evaluate the conditional. */
-
-	if (ROFF_el == tok) {
-		/* 
-		 * An `.el' will get the value of the current rstack
-		 * entry set in prior `ie' calls or defaults to DENY.
-	 	 */
-		if (r->rstackpos < 0)
-			rule = ROFFRULE_DENY;
-		else
-			rule = r->rstack[r->rstackpos];
-	} else
-		rule = roff_evalcond(*bufp, &pos);
+	rule = ROFF_el == tok ?
+		(r->rstackpos < 0 ? 
+		 ROFFRULE_DENY : r->rstack[r->rstackpos--]) :
+		roff_evalcond(*bufp, &pos);
 
 	sv = pos;
-
 	while (' ' == (*bufp)[pos])
 		pos++;
 
@@ -1018,16 +1005,20 @@
 
 	r->last->rule = rule;
 
+	/*
+	 * An if-else will put the NEGATION of the current evaluated
+	 * conditional into the stack of rules.
+	 */
+
 	if (ROFF_ie == tok) {
-		/*
-		 * An if-else will put the NEGATION of the current
-		 * evaluated conditional into the stack.
-		 */
-		r->rstackpos++;
-		if (ROFFRULE_DENY == r->last->rule)
-			r->rstack[r->rstackpos] = ROFFRULE_ALLOW;
-		else
-			r->rstack[r->rstackpos] = ROFFRULE_DENY;
+		if (r->rstackpos == RSTACK_MAX - 1) {
+			mandoc_msg(MANDOCERR_MEM, 
+				r->parse, ln, ppos, NULL);
+			return(ROFF_ERR);
+		}
+		r->rstack[++r->rstackpos] = 
+			ROFFRULE_DENY == r->last->rule ?
+			ROFFRULE_ALLOW : ROFFRULE_DENY;
 	}
 
 	/* If the parent has false as its rule, then so do we. */