On 3 Dec, Sven Neumann wrote:
> you recently checked in a huge change to GIMP that we (Mitch and me)
> have been very unhappy with. In the meantime, I have gone through the
> hassle of looking at every single line of your changes and I have
> reverted most of it. Here are the reasons, why we think your
> "optimizations" don't make sense and, even worse, are dangerous:
BTW: It would have been a single command for me to revert the changes
but since you decide to work everything on your own.
> While unsigned variables can speed things up under very special
> circumstances,
Not special, in many 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.
> 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.
>> use bitfields in structs where possible
> this is an optimizations of structs sizes. It does make sense to use
> bitfields for boolean fields if large numbers of this struct are
> allocated. For rarely used objects the gain in memory usage is
> neglible however.
It's not only the memory consumption, it's also the addressing which is
much faster (normally) for bitfields because one can read several states
at once while the access is realised with masking instead of several
memory reads (or writes) and as such using bitfields has several huge
advantages:
- Less memory accesses (and all the results thereof)
- Smaller objects because of less code
- Implied error checking against abuse by the compiler
My only worry here was that some compilers don't support them but since
you started using them in other files I didn't see any problem with
this.
> Since the change makes the code less readable it should be applied
> with care.
I disagree strongly here. The code itself doesn't change and structure
definitions tend to win when it comes to readability here.
> The change again also bears the possibility of introducing bugs.
I see it the other way. It bears the possibility to detect unseen bugs.
> This happens when values others than TRUE (1) and FALSE (0) are
> assigned to struct fields defined as unsigned int : 1.
What do you expect to happen then?
> For this reason, this change should only be done
> late in the development cycle. Once the API is settled and the code is
> stable, we can think about converting booleans in frequently used
> structs to bitfields. At the very moment, it doesn't make sense at
> all.
Well, your thought.
> you changed code from
> gimp->be_verbose = be_verbose ? TRUE : FALSE;
> to
> gimp->be_verbose = be_verbose;
> This is wrong, especially if a bitfield is used. I don't think I need
> to explain the reason.
It doesn't even make sense when NOT using bitfields but I definitely see
no problem when doing so. Please don't always assume I see what you mean
by telling "this is wrong"; if you see a problem here tell me so.
> 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.
You not being able to use CVS efficently doesn't make me a bad person.
Maybe you would have saved you a lot of time if you simply asked me
to revert the (still correct) changes.
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.
BTW: Not that I've never corrected any of yours or Mitches code
resulting from a thinko or type...
It's up to you.
--
Servus,
Daniel
