![]() | |||||||||||||
Messages in this thread |
On Wed, 7 Jan 2004, Linus Torvalds wrote: > > On Thu, 8 Jan 2004, Jesper Juhl wrote: > > > > The 'arg' argument to the function do_fcntl in fs/fcntl.c is of type > > 'unsigned long', thus it can never be less than zero (all callers of > > do_fcntl take unsigned arguments as well and pass on unsigned values), > > I'm not sure I like these kinds of patches. > Ok, let me try and argue in favour of it, and if you think the arguments are bogus then I won't be doing any more of this type of patches. > I _like_ the code being readable. I can't argue with that, but I don't think this patch actually decreases readabillity. It's still perfectly clear what the remaining code does, and if anybody is wondering if 'arg' could ever be <0 then a quick glance at the type will answer that. Would you like this sort of patch better if removing the code went hand-in-hand with the addition of a one-line comment stating something like /* the test for arg < 0 is not done since arg is unsigned */ or ? > The fact that the compiler can optimize > away one of the tests if the type is right i2Ds fine. It seems to be > draconian to remove code that is correct and safe, especially when the > code has no real downsides to it. From my point of view it's a matter of correctness. Testing an unsigned value for <0 makes no sense, and doing things that make no sense is a bad habbit in my oppinion. Yes, the code will be optimized away, so it doesn't actually do any harm, and in this case it's a very small amount of code, but that's not always so. A little while ago I posted a similar patch that removes some dead code in (amongst others) ReiserFS, and in that case it's a bit more code (not much, but a bit more) and there I think removing the impossible code makes the code easier to read since a person trying to find out what's going on does not have to spend time tracing the workings of something that can never execute in any case. > > Do we have a compiler that needlessly complains again? > Gcc /will/ warn about the fact that the result of an unsigned comparison with <0 is always false if the code in question is compiled with "-W -Wall", with the standard compile options no such warning is given. > Sometimes it is the _complaints_ that are bogus. > I'm not arguing against that. My mission here is not to silence any warning just for the sake of silencing the warning, although I must admit that I am trying to get rid of as many potential warnings as I can - It would be nice to be able to compile the kernel with "-W -Wall" and not have the output too cluttered, and some of the things that gcc will warn about could potentially hide real bugs, so I believe it's a valid exercise. But my real goal is mainly to find and fix potential problems, squishing potential warnings is a secondary bennefit. -- Jesper Juhl - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ | ||||||||||||
| Last update: 2005-03-22 12:59 [from the cache] ©2003-2008 | |||||||||||||