On Mon, 3 Dec 2001 degger@xxxxxxx wrote: > On 3 Dec, Sven Neumann wrote: > BTW: It would have been a single command for me to revert the changes When Gimp first moved to CVS, and access to the source tree went from a strong central maintainer to many people with CVS access, the reason I was told that it would work is that if something is committed that the community agrees is not what it wants, the whole of the patch can be reverted with a single command. > > While unsigned variables can speed things up under very special > > circumstances, they bear the danger of introducing subtle bugs that > > are almost impossible to find. A good example is found in > > pango/docs/TEXT/coding-style (a recommened read, btw.): > > > If width is unsigned and 10, then: > > > > int new_width = MAX (width - 15, 1); > > > > produces 4294967291, not 1. > > This is clearly a bug in the code. There are sizes which are by > definition unsigned and code computes an negative value this is > about as harmful as an big positive one. I really want to talk more about the political issues than the technical ones, so I will limit my comments. This may be a bug when used with unsigned numbers, but it certainly is a valid and acceptable approach to go out of range with signed numbers, which is what the program was using. Since the MAX macro returns a 1 for all nonpositive numbers, it's not a problem. If this weren't ok, why would we need the CLAMP macro? Always staying in range can be a difficult and inefficient way of doing things. Changing signed to unsigned turns this nonbug into a bug. While it is possible to rewrite this particular example by using a (hypothetical) function saturated_subtraction(width, 15, 1) which has some nastiness to ensure that width never goes negative, I personally find that to be a less readable way of doing things. (although such a function could be useful at times.) It could also be implemented as a macro, although the amount of nastiness required for doing that is scarcely imaginable. > > Also, in lots of places you've changed, the value -1 was used to > > indicate an unset value (width and height of GimpPreview for > > example). > > Shouldn't be. I was about to go even further and change all false uses > of such values. What you imply is that using unsigned types for unsigned > values is wrong because you also use them for errorindication and this > (together with magic constants) is about most of the evilness of > programming. How do you propose to do so? Add a "valid" bit to each structure for each varible used in this way? There go any space savings you might have had. I personally find nothing wrong with signifying an invalid value with an invalid value. While you do run into problems when the value you use to signify an invalid value is a valid value, this will never be the case with things like width (which will never, ever be negative). This is a very accepted practice in C, and even in places and languages that try avoid such usage end up doing it with pointers. I've only seen one language that doesn't have an equivalent to "NULL", the pointer that isn't a pointer -- the invalid pointer. I fail to see why using NULL to signify an invalid value for a pointer is any different than using negative numbers to signify an invalid "unsigned" number. > > Daniel, would you please from now on inform us about the changes you > > are working on and send patches to this mailing list instead of > > applying your work to CVS without asking. It took me hours to revert > > your changes and I don't want to have to go through this again. If you > > think I reverted too much (which I'm sure has happened in a few > > places), please send the relevant changes in small patches so we can > > discuss them here. > > Anyway, since you seem to like to develop on your own because you know > everything better anyway; feel free to do so. I don't care whether > correct code shines off your ass or you have the Saft but putting > embargos up to me because you don't understand what I'm trying to > achieve is just likely to piss off another of the few coders left. I don't think that Daniel's reaction was inappropriate at all. Yet because I'm late for class, I'll let what was said stand for itself. What I do think is important is that all major changes (yes, even major changes by Sven and Mitch) should be discussed before commiting (and even better, before a significant amount of effort is invested). Preferably, it should be discussed on both the mailing list and IRC, with any relevant points made on IRC echoed to the newsgroup (we all get mail and can read it at our convience, but only Yosh reads the stuff in #gimp 24/7) I admit to being as bad about this in the past as anyone else here. Consider this notification that I will change. I don't know how many times the first time I've heard about some massive change to the code for the first time in the Changelog, which is usually something very discriptive like: * some/file.[ch]: reorganized crap beyond all recognition (I am exaggerating somewhat. Alas, only somewhat.) Personally, I can't think of many things that can be done to discourage developers than this. I can't count the number of times something I have been working on has been broken by some massive unanounced change. I cringe to cvs update when I'm working on something, but I do just because to not would be to just postpone the inevitable. Honestly, almost all of the big changes that have been made are good and uncontriversial -- a simple notification beforehand would be fine. I certainly don't want to disparge those that work actively on Gimp, especially Sven and Mitch. The contributations that all have made are very valuable. But I have to wonder if Sven and Mitch don't consider Gimp to have become thier own personal toy, and this overly harshly toned critism of Daniel's code is a good example of this. Continuing this pattern will do nothing to further Gimp in any real way, but will serve quite well to drive away the few developers that Gimp has left. And to think that only a handful of months ago I wondered why the excitment and number of gimp developers had died down so much... Rockwalrus
