Re: term.c and lint

[Kristaps Dzonsons <kristaps@bsd.lv>]

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