Hi, degger@xxxxxxx writes: > > but you continue to state that it makes the code cleaner which it > > clearly doesn't. > > I say using a type that represents the actual type of the value > closely is a feature and not a bug. What wrong about seing: > Hey, this value is supposed to be unsigned? > This is a cleanup like using enums instead of magic values. if we'd use a programming language that would properly support ranged types, it'd be a cleanup. C however doesn't assure that. If the program would throw a signed exception if a negative value is ever assigned to an unsigned variable, we'd indeed be able to catch bugs and declaring variables as unsigned would make sense. Since this is not the case, the use of unsigned variables is dangerous without giving any real advantages. > > it gives a small improvement and is probably a valid optimization in > > this place. > > That's also not the point. You doubted that using unsigneds/signeds > doesn't make a difference and I told you that it might not make a > difference on all platforms and compilers but definitely does in not > so few cases. your example involved a modulo operation. Modulo is only properly defined on positive values so it definitely makes sense to use an unsigned variable here. But then modulo operations are rare, subtractions are much more frequent and they will give unexpected results if unsigned variables are involved. > > The difference to the changes I reverted is that this is a > > very local change. The function is small and the variable is local. It > > is easy to decide that using an unsigned integer here does not have > > any unwanted side effects. > > Sure, but since you're breaking GIMP anyways what's the problem with > doing it correctly here? BTW: I tested the afffected functions > (including the brushes that you stated would break) and at least there > is no obvious breakage but if someone is careing about the code it can > only get better not worse. you can't have tested all possible usage patterns since there are way too many. > > Your changes however did affect mostly headers. This means the values > > are used in a lot of places and it is almost impossible to check them > > all. Even if you would check them all, a future change to the code > > might introduce a bug because the one who did the change didn't think > > about the unsignedness of the variable (which is defined somewhere > > else). > > It will introduce bugs but it will also make a few older bugs obvious > and correctable. the point is: how are we supposed to catch them? If the compiler would give us hints like it does for wrong usage of consts and enums, great. However, this is not the case unfortunately. > > Since the code is very much in flux right now, a lot of these > > changes are going to happen in the future. > > I don't get it. distances are for instance per definition always > positive if we have a concrete list of types that are always signed > or unsigned and we stick to it then we'd have more then now: > A documentation and guide for datatypes. distances are a good example since they can very well be negative: dist = a - b; this perfectly valid code will awfully break if b is larger than a and dist is defined unsigned. For the CPU and the compiler it wouldn't make any difference, only the outcome would be wrong. > > In the worst case the problem only becomes apparent under rare > > circumstances and is not even caught during the development process. > > Yes, this might happen though I consider that unlikely. then have a look at some of the bugs reported in plug-ins lately. They have been reported years after the first release and didn't get caught because they only showed up when the plug-in was used on small images (width < 64 || height < 64). Such a bug could very well be caused by the use of an unsigned variable. Salut, Sven
