Hi, degger@xxxxxxx writes: > 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 yes! > 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
