Re: term.c and lint

[Ingo Schwarze <schwarze@usta.de>]

Hi Kristaps,

Kristaps Dzonsons wrote on Mon, Sep 27, 2010 at 11:20:44AM +0200:

> I'm completely with you on this one.  OpenBSD's lint is overzealous,
> especially when indexing with size_t.  It's ridiculous:  malloc and
> company /demand/ size_t when allocating memory---yet the same type
> can't be used to iterate over the array?  Faugh.
> 
> Please leave the code as-is, in this case: I just wanted to be sure
> that they warnings are benign.

Hm, i think part of these changes actually make the code easier to
understand.  I mostly freaked out on those that don't seem to allow
any sane way to please lint, and of course on the indexing issue.

> I'll probably slowly cast out (har, har) explicit casts used to quiesce
> lint.  The same goes with those absurd pre-loop LINTED comments.

Yes, i think getting rid of most of the LINTED comments is good,
because they don't even tell you what is special about the next line.
I also think that explicit casts when indexing arrays is rather ugly.

But when you do indeed cast from one type to another, not trivially
compatible type, making the cast explicit does not seem that bad,
it provides a hint that something potentially dangerous is happening
at that place.  As long as we have many such casts in our code,
i'm not sure having them implicit and lint complain is better
than having them explicit.  Rather, i'm wondering whether there
are techniques to avoid such problems in the first place, in the
design stage or whenever...

That said, i have thrown out those parts of my term.c diff that
seemed silly to me.  I think the rest makes sense in any case.
As it is tested, i could as well put it in, right?

This is what it does:
 * make the initial maxvis/mmax calculation easier to understand
 * where real, non-indexing casts happen, make them explicit
 * avoid a few lint warnings that can easily be fixed
 * remove one needless LINTED comment

Yours,
  Ingo


Index: term.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/term.c,v
retrieving revision 1.52
diff -u -p -r1.52 term.c
--- term.c	1 Oct 2010 21:38:26 -0000	1.52
+++ term.c	1 Oct 2010 21:59:48 -0000
@@ -142,24 +142,21 @@ term_flushln(struct termp *p)
 	 * an indentation, but can be, for tagged lists or columns, a
 	 * small set of values. 
 	 */
-
-	assert(p->offset < p->rmargin);
-
-	maxvis = (int)(p->rmargin - p->offset) - p->overstep < 0 ?
-		/* LINTED */ 
-		0 : p->rmargin - p->offset - p->overstep;
-	mmax = (int)(p->maxrmargin - p->offset) - p->overstep < 0 ?
-		/* LINTED */
-		0 : p->maxrmargin - p->offset - p->overstep;
+	assert  (p->rmargin > p->offset);
+	dv     = p->rmargin - p->offset;
+	maxvis = (int)dv > p->overstep ? dv - (size_t)p->overstep : 0;
+	dv     = p->maxrmargin - p->offset;
+	mmax   = (int)dv > p->overstep ? dv - (size_t)p->overstep : 0;
 
 	bp = TERMP_NOBREAK & p->flags ? mmax : maxvis;
 
 	/*
 	 * Indent the first line of a paragraph.
 	 */
-	vbl = p->flags & TERMP_NOLPAD ? 0 : p->offset;
+	vbl = p->flags & TERMP_NOLPAD ? (size_t)0 : p->offset;
 
-	vis = vend = i = 0;
+	vis = vend = 0;
+	i = 0;
 
 	while (i < (int)p->col) {
 		/*
@@ -180,7 +177,6 @@ term_flushln(struct termp *p)
 		 * space is printed according to regular spacing rules).
 		 */
 
-		/* LINTED */
 		for (j = i, jhy = 0; j < (int)p->col; j++) {
 			if ((j && ' ' == p->buf[j]) || '\t' == p->buf[j])
 				break;
@@ -219,8 +215,7 @@ term_flushln(struct termp *p)
 
 			/* Remove the p->overstep width. */
 
-			bp += (int)/* LINTED */
-				p->overstep;
+			bp += (size_t)p->overstep;
 			p->overstep = 0;
 		}
 
@@ -234,7 +229,7 @@ term_flushln(struct termp *p)
 				j = i;
 				while (' ' == p->buf[i])
 					i++;
-				dv = (i - j) * (*p->width)(p, ' ');
+				dv = (size_t)(i - j) * (*p->width)(p, ' ');
 				vbl += dv;
 				vend += dv;
 				break;
@@ -283,8 +278,7 @@ term_flushln(struct termp *p)
 
 	if (TERMP_HANG & p->flags) {
 		/* We need one blank after the tag. */
-		p->overstep = /* LINTED */
-			vis - maxvis + (*p->width)(p, ' ');
+		p->overstep = (int)(vis - maxvis + (*p->width)(p, ' '));
 
 		/*
 		 * Behave exactly the same way as groff:
@@ -298,8 +292,7 @@ term_flushln(struct termp *p)
 		 */
 		if (p->overstep >= -1) {
 			assert((int)maxvis + p->overstep >= 0);
-			/* LINTED */
-			maxvis += p->overstep;
+			maxvis += (size_t)p->overstep;
 		} else
 			p->overstep = 0;
 
@@ -307,9 +300,8 @@ term_flushln(struct termp *p)
 		return;
 
 	/* Right-pad. */
-	if (maxvis > vis + /* LINTED */
-			((TERMP_TWOSPACE & p->flags) ? 
-			 (*p->width)(p, ' ') : 0)) {
+	if (maxvis > vis +
+	    ((TERMP_TWOSPACE & p->flags) ? (*p->width)(p, ' ') : 0)) {
 		p->viscol += maxvis - vis;
 		(*p->advance)(p, maxvis - vis);
 		vis += (maxvis - vis);
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv