> However I see much room for improvement here. The values I changed
> to unsigned are likely meant to be unsigned like sizes, distances or
> heights or widths. And those are frequently used in loops like
> int i;
> for (i = 0; i < width; i++)
>   foo ();
> if we were to expect negative values in this case we'd be really doomed
> and as such I consider it a feature to be warned by the compiler if a
> calculation is definitely underflowing or tests are useless.
> Negative values do have a right to exist when it comes to coordinates
> but using signed types for error reporting AND value represantation
> at the same time is likely to be errorprone.

Using them for error reporting is definitely a bad idea. Using a negative
value to indicate that a value has not been set and needs to be computed
is IMO a reasonable usage.

> I propose to do it the following way:
> gboolean do_something (guint *value, ...)
> (means passing a pointer to the place the result should be written to)
> instead of 
> gint do_something (...)
> and returning a negative value in case of failure


> I've applied exactly this scheme to tile_manager_get_tile_num () in 
> tile-manager.c and together with replacing the ongoing errorchecking
> throughout the callees was able to save a whooping 480 bytes in object
> size on PPC and got faster code while (IMHO) making the code clearer.

you also removed a good bunch of debugging code that might still be
needed (Kelly is working on this code) and "unsignified" lots of
integers, which is why I choose to revert the changes in this file in
a whole. I do hope you will post the tile_manager_get_tile_num()
part of your change to the list.

> I do because one has to constantly check for errorvalues and there 
> lies one problem; we have several places in the GIMP where return 
> values are checked for errors and then passed down to another function
> where they are checked again while there are also places where we don't
> have error checking at all.

Do we? We do have a lot of g_return_val_if_fail() statements all over
the place, but this is for debugging purposes only and should be
disabled for production code. No such call should ever be triggered;
if it is, there's a bug. For this reason, the caller does not need to
check for values returned by g_return_val_if_fail(). The whole purpose
of these statements is to catch bugs early.

If code makes assumptions about parameters (like for example assuming
that width is > 0), there has to be code like g_return_if_fail() to
check this assumption. Defining width as an unsigned integer would
also guarantee this, but it unneededly introduces a possibility for a
bug if width is ever used in a subtraction.

> Using signed types is unelegant and prevents the compiler from
> optimising the code better. NULL pointers have the nice property of
> being easy to check and I think it's a nice idea to have functions
> return 0 or !0 aka TRUE or FALSE instead of some bogus values
> because it's clearer and faster.

I agree about the return value thing, but I doubt the use of signed or
unsigned makes up for any noticeable speed difference expect under
very rare conditions. Before any integer is changed to an unsigned
value, a benchmark should proove that it really is an optimization and
all code using the value needs to be reviewed carefully.

Salut, Sven

Gimp-developer mailing list

Reply via email to