From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp-2.sys.kth.se (smtp-2.sys.kth.se [130.237.32.160]) by krisdoz.my.domain (8.14.3/8.14.3) with ESMTP id o8R9KtXI024175 for ; Mon, 27 Sep 2010 05:20:56 -0400 (EDT) Received: from mailscan-1.sys.kth.se (mailscan-1.sys.kth.se [130.237.32.91]) by smtp-2.sys.kth.se (Postfix) with ESMTP id D924414F3BF; Mon, 27 Sep 2010 11:20:49 +0200 (CEST) X-Virus-Scanned: by amavisd-new at kth.se Received: from smtp-2.sys.kth.se ([130.237.32.160]) by mailscan-1.sys.kth.se (mailscan-1.sys.kth.se [130.237.32.91]) (amavisd-new, port 10024) with LMTP id DXvUacnXEtXD; Mon, 27 Sep 2010 11:20:47 +0200 (CEST) X-KTH-Auth: kristaps [193.10.49.5] X-KTH-mail-from: kristaps@bsd.lv Received: from [172.16.18.84] (unknown [193.10.49.5]) by smtp-2.sys.kth.se (Postfix) with ESMTP id 7B7F914F3B6; Mon, 27 Sep 2010 11:20:45 +0200 (CEST) Message-ID: <4CA061EC.1070006@bsd.lv> Date: Mon, 27 Sep 2010 11:20:44 +0200 From: Kristaps Dzonsons User-Agent: Mozilla-Thunderbird 2.0.0.24 (X11/20100329) X-Mailinglist: mdocml-tech Reply-To: tech@mdocml.bsd.lv MIME-Version: 1.0 To: Ingo Schwarze , "tech@mdocml.bsd.lv" Subject: Re: term.c and lint References: <4C9F1241.9040706@hhs.se> <20100926172709.GC3502@iris.usta.de> In-Reply-To: <20100926172709.GC3502@iris.usta.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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