On 5 Dec, Sven Neumann wrote:
> 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.
And I also say one variable per purpose because everything else is hard
follow and likely to break though my patch contained just one patch
to address that.
> 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.
> 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.
> 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.
> 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.
> It is definitely too early to do such global optimizations and I even
> vote for never doing them.
Wow.
> Using unsigned variables locally in parts of the GIMP core that
> are executed frequently is still a valid option.
Interesting, how do you come to this conclusion? :)
> no, it breaks things in a very different manner: the compiler will now
> warns us if we try to modify values that we aren't supposed to modify.
> Thus we catch bugs at compile time. The compiler will however not warn
> us if we use unsigned variables in a way that might cause the value to
> become negative.
Wrong, if you make it obviously wrong it will issue warnings. If we do
it secretly wrong then we'll experience that because it's not of by 1 or
by 10 but by >20bit and this is easy to track down.
> The result here will be a mis-behaving application
> with no indication of where we should look for the problem.
I disagree. Obvious problems are much easier to find then sneaky hidden
ones.
> 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.
> the directories that contain pure UI code are called gui and widgets.
> Is that not simple enough? The point however is not to disallow
> optimizations in the UI and allow them everywhere else. Most parts of
> the core are unfinished yet and shouldn't be optimized at this stage.
And I tell you again that it's not only optimisation.
> we obviously disagree on code cleanliness. IMO the use of unsigned
> variables does not make the code any cleaner.
Seems so.
--
Servus,
Daniel
