Re: term.c and lint

Re: term.c and lint

[Kristaps Dzonsons]

Ingo, note cross-posting to tech@, just so we're all on the same page...

>> Can you please run "make lint LINTFLAGS='-cefuhx'"
> 
> Sure, i have just done so.
> 
>> and see if the lint warnings in term.c are bad?
> 
> It looks like none of them indicate bugs.
> 
>> I don't think they are, and if you can verify this, please add the
>> necessary /* LINTED */ magic to quiesce the warnings...
> 
> I'm rather unhappy about this.
> 
> On the one hand, i agree that using automatic code checkers like lint
> is good, even if i tend to neglect it, and i agree that getting rid of
> any warnings is required, or we end up missing real bugs in the noise.
> 
> But i'm still unhappy for two reasons.
> 
> First, i have a feeling that the amount of lint goo we are adding
> to the code in unreasonable, i feel it is just too much.  Such casts
> and comments can hide bugs, even if somebody checked them once
> upon a time.  So they should be minimized.
> 
> Besides, there are some lint warning that i plainly don't know how
> to fix without adding exceedingly ugly code.
> 
> For example, take this one in term.c, function term_word():
> 
> 	const char	*word;
> 	size_t		 ssz;
> 	/* ... */
> 	word += ssz;
> 
> Lint complains:
> 
> 	"converted from 'unsigned long' to 'long'"
> 
> Now that doesn't make much sense in the first place.
> Unless i'm quite mistaken, the size_t type has been
> introduced explicitely to deal with lengths of arrays
> in memory.  So, moving a pointer by a size_t looks
> like one of the sanest conceivable uses of a size_t.
> 
> Yes, i know that it was a language design error to define size_t
> as unsigned; it should have been signed in the first place,
> but deploring that doesn't mend matters.
> 
> So, what shall i do?
> 
> 	word += (int)ssz;
> 	word += (signed)ssz;
> 
> That helps, but is plainly wrong.
> In principle, ssz might be larger than an int.
> 
> 	word += (long)ssz;
> 
> That helps, but is wrong as well.
> Who says that size_t is the same size as long?
> That is platform dependent and not portable.
> 
> 	/* LINTED */
> 	word += ssz;
> 
> That is already somewhat ugly, and doesn't even help!
> 
> 	word += /* LINTED */
> 	    ssz;
> 
> That does help, but it is positively revolting.
> 
> I mean, part of the art of programming is to make the code
> easily understandable to humans.  That is also relevant
> for correctness:  When the code is ugly or hard to understand,
> bugs will not be found.
> 
> 	word = &word[ssz];
> 
> Again, rather ugly and doesn't even help.
> 
> 	word += (ssize_t)ssz;
> 
> That does help, as POSIX guarantees that ssize_t is unsigned,
> but unfortunately, POSIX does not guarantee that size_t and ssize_t
> are of the same size, so it is not correct either.
> 
> So i am really stumped for any acceptable way to fix this.
> 
> I also looked for examples in the OpenBSD tree and found some
> in /usr/src/lib/libc/, e.g. string/memrchr.c, stdlib/realpath.c,
> stdlib/merge.c - all of these just have
> 
> 	word += ssz;
> 
> and let lint complain.
> 
> I'm very unhappy because i have a suspicion there must be sane
> practices to deal with these issues, good styles and idioms and
> designs you can use to encounter smaller numbers of them in the
> first place, but i just know too little about them and have no
> idea where to look for reading up on best practices.
> 
> Basically, i suspect this is about getting advanced code quality -
> not regarding the big picture, but regarding the gory details.
> I think i know a bit in that respect in Perl, but i lack that
> knowledge regarding C, and i have no idea where to start
> reading about it.
> 
> What we are doing now, putting lots of casts and /* LINTED */
> all over the place looks very suspicious to me...   :-(
> 
> I guess i will try to ask some of the OpenBSD hackers during
> EuroBSDCon in Karlsruhe, let's see what reading they recommend...
> 
> So, here is the best i have right now.
> What i did:
>  - Prefer explicit casts to /* LINTED */ where possible:
>    They do exactly the same thing and help understanding.
>  - Remove two pointless /* LINTED */ comments.
> 
> But i *hate* part of it, in particular the /* LINTED */
> on the same line as the += ...

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.

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

Thanks,

Kristaps
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: term.c and lint

[Ingo Schwarze]

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

Re: term.c and lint

[Kristaps Dzonsons]

>> 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

Yes, I like this---much cleaner.  Please commit.

Thanks!

Kristaps

--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv