Cleanup patches - comparison is always [true|false] + unsigned/signed compare, and similar issues. (consolidating existing threads)

Cleanup patches - comparison is always [true|false] + unsigned/signed compare, and similar issues. (consolidating existing threads)

[Jesper Juhl]

Since I see people commenting, on what is essentially the same issue, in
3 or four different threads, I thought I'd try to consolidate the
discussion in a completely new thread - since it's moved beyond concrete discussion
of the actual patches I posted and into a discussion about that sort of
patches in general.
I'm CC'ing the people who have been commenting on the previous threads -
those threads being those I started with the topics :

"Suspected bug infilesystems (UFS,ADFS,BEFS,BFS,ReiserFS) related to sector_t being unsigned, advice requested"
"[PATCH] mm/slab.c remove impossible <0 check - size_t is not signed - patch is against 2.6.1-rc1-mm2"
"[PATCH] fs/fcntl.c - remove impossible <0 check in do_fcntl - arg is unsigned."
"[PATCH][RFC] variable size and signedness issues in ldt.c - potential problem?"

As I see it the, issue is patches that clean up 'comparisons that are
always true or false', 'signed vs unsigned comparisons' and similar
"minor" issues.


First let me try to state clearly what my purpose(s) in creating those
patches, and the patches I had originally intended to follow them, were.
And also what I did /not/ intend to do with those patches.


A) I wanted to remove code that could potentially be hiding a real bug.
Examples would be code that by using an unsigned value might be casting
away a needed 'signedness' of a variable. Code that by mixing signed and
unsigned comparisons might be buggy in the case where the signed value
overflows and all other nasties that can potentially happen when signed
and unsigned values are mixed.

B) I wanted to remove dead code that was never being executed under any
circumstances. This includes both tiny fragments and larger sections of
code. I wanted to do this for 2 main reasons; first of all to attempt to
cut down the amount of unnessesary code for people to read through when
trying to understand the workings of a certain bit of code.
Secondly to try find cases where changes to code had left some bits
obsolete but those bits had been left in by mistake.

C) Remove as many warning messages as possible. One reason is to get less
cluttered output in general and also when compiling with -W. Another
reason being that some warnings actually indicate real bugs - I'm aware
that some warnings are completely pointless and the code being warned
about is exactely as it should be, and such warnings should be ignored.


What I did not intend to do with those patches was to decrease the
readabillity of the code or trouble maintainers with pointless
patch-reading.


People have been commenting both in favour of the patches and against
them, and my main reason for starting this new thread is to try and find
out if I should continue with this work or not.
I don't want to bother anyone with pointless patches, and I see no need to
spend my time doing stuff that people don't appreciate either, but if some
sort of agreement can be reached on what (sub)set of this type of patches
are wanted, then I'll be happy to do the work to track the code locations
down and provide such patches.


Thank you for reading this far.


-- Jesper Juhl

Re: Cleanup patches - comparison is always [true|false] + unsigned/signed compare, and similar issues. (consolidating existing threads)

[Tim Cambrant]

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

On Thu, Jan 08, 2004 at 06:59:48PM +0100, Jesper Juhl wrote:
> 
> A) I wanted to remove code that could potentially be hiding a real bug.
> Examples would be code that by using an unsigned value might be casting
> away a needed 'signedness' of a variable. Code that by mixing signed and
> unsigned comparisons might be buggy in the case where the signed value
> overflows and all other nasties that can potentially happen when signed
> and unsigned values are mixed.

This should always be pursued. Unnecessary code does infact still exist
inside the kernel, and if a bug should find it's way in there it would
both be annoying for developers and dangerous for end-users.

> B) I wanted to remove dead code that was never being executed under any
> circumstances. This includes both tiny fragments and larger sections of
> code. I wanted to do this for 2 main reasons; first of all to attempt to
> cut down the amount of unnessesary code for people to read through when
> trying to understand the workings of a certain bit of code.
> Secondly to try find cases where changes to code had left some bits
> obsolete but those bits had been left in by mistake.

Readability should always be an issue, since the amount of documentation
and how easy a newbie can find his/her way through the sources is really
important for Linux. The easier it is to understand the code, the more
people will get involved in kernel developing in the end. It also sends
a message of professionality to have clean code which is consistant and
solid. From personal experience, I have become quite worried and paranoid
about the Linux kernel and software in general since I began reading LKML
and following up bugs, since issues like this do exist. Not that I have
ever had any real problems at all with the kernel, but knowing that a
lot of the code is unmaintained and erroneous takes away some of the
feeling of stability and security that the initial impression of Linux
gave me. Not that this has much to do with actual developing, but since
I am not a developer, I think I speak for most end-users who dig a little
deeper than desktop usage of Linux.

> C) Remove as many warning messages as possible. One reason is to get less
> cluttered output in general and also when compiling with -W. Another
> reason being that some warnings actually indicate real bugs - I'm aware
> that some warnings are completely pointless and the code being warned
> about is exactely as it should be, and such warnings should be ignored.

Warnings can infact be the problems with the compiler, and not errors
in the code itself. In some cases the programmers do the right thing,
and the compiler-programmers has to adjust to what infact is the right
thing to do. With this being said, warnings _are_ annoying. Someone
ought to categorize different types of warnings and only remove the
ones that everyone (or someone with authority) agrees is safe to
correct. Removing warnings just for the fun of it might not always
be the way to go.
 
> What I did not intend to do with those patches was to decrease the
> readabillity of the code or trouble maintainers with pointless
> patch-reading.

Most maintainers would benefit from these patches in the end, no
matter how annoying it is to read through them at first. It is a
kernel janitor job, but even the "important" and busy developers
should care about these things. Nobody wants a messy kernel, so
a cleanup process like this would probably be appreciated from most
people.

> I don't want to bother anyone with pointless patches, and I see no need to
> spend my time doing stuff that people don't appreciate either

Indeed, it's not fun to do something that no-one appreciates, so a
rally like this might be necessary to get people to notice that a
project like this is going down. Posting cleanup-patches once a week
to LKML is probably not the way to go, since most of them will be
ignored and/or flamed.

Great initiative, Jesper. Cleanups are needed too. Being a fan of
cleanliness and neat code I suggest you go ahead if people seem
to care about these things.


                Tim Cambrant

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Cleanup patches - comparison is always [true|false] + unsigned/signed compare, and similar issues. (consolidating existing threads)

[Mike Fedyk]

On Thu, Jan 08, 2004 at 08:25:39PM +0100, Tim Cambrant wrote:
> project like this is going down. Posting cleanup-patches once a week
> to LKML is probably not the way to go, since most of them will be
> ignored and/or flamed.

Use the trivial patch monkey for this, it will save you a lot of headache.

Re: Cleanup patches - comparison is always [true|false] + unsigned/signed compare, and similar issues. (consolidating existing threads)

[Jesper Juhl]

On Thu, 8 Jan 2004, Mike Fedyk wrote:

> On Thu, Jan 08, 2004 at 08:25:39PM +0100, Tim Cambrant wrote:
> > project like this is going down. Posting cleanup-patches once a week
> > to LKML is probably not the way to go, since most of them will be
> > ignored and/or flamed.
>
> Use the trivial patch monkey for this, it will save you a lot of headache.
>
Good idea. I'll feed patches there, and only there unless someone
expresses a wish to have them send to lkml as well.


-- Jesper Juhl

Re: Cleanup patches - comparison is always [true|false] + unsigned/signed compare, and similar issues. (consolidating existing threads)

[Paul Jackson]

The key question in my view was what code was easiest to understand.
This is closest to being the code that is shortest, stripped of all
non-essential detail.  But not exactly.  Code is more like novel or
essay in my view, than a poem.  I don't find haiku clear.  However,
what is easiest to understand is a judgement call.

Since we were seeing here, with the remarks of folks such as myself,
a nice example of that well known phenomenon where a committee will
debate for hours over the $25 budget line item, and then pass the
$3 million item without comment, your conclusion to try using the
Trivial Patch Monkey sounds like a winner.  Rusty has good judgement,
and for changes such as this, better one good judge making immediate
decisions, than lengthy lkml threads.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373