[GRASS-dev] lib fn to convert int color number to RGB values?

Glynn Clements glynn at gclements.plus.com
Fri Feb 1 07:04:11 EST 2008


Ivan Shmakov wrote:

>  >>> The former is only an advantage if there's a situation where a
>  >>> module doesn't just call G_fatal_error() when an invalid colour
>  >>> occurs. Given that invalid user input is usually a fatal error, I
>  >>> can't immediately think of a situation where you might get an
>  >>> invalid colour string without it resulting in a fatal error.
> 
>  >> Did you consider the use of the function from within a GUI
>  >> application?  Hardly calling exit () on incorrect input from the
>  >> user is a friendly behaviour for a GUI application.
> 
>  > GRASS simply doesn't work in such a context. One more function
>  > doesn't make any difference unless you're going to fix the thousands
>  > that already behave this way.
> 
> 	Well, I haven't looked at the code just yet, but it seems that
> 	the references to the functions in question aren't scattered too
> 	widely across the library:
> 
> $ find lib/ -name \*.o -print0 \
>       | xargs -r0 -- nm -- \
>       | grep -E 'U (_?exit|G_fatal_error)$' \
>       | LC_ALL=C sort | uniq -c 
>      97          U G_fatal_error
>       4          U _exit
>      39          U exit
> $ find lib/ -name \*.o | wc -l 
> 811
> $ 
> 
> 	(Yes, there're probably a number of indirect callers as well.)

Yes, there are a lot of indirect callers. That's largely the point:
individual modules don't have to worry about handling errors, as many
functions either succeed or die.

>  > BTW, we seem to have had this discussion at least once a year for as
>  > long as I have been subscribed to these lists (~7 years).
> 
> 	I'm glad that I'm not the first one to discover that there's a
> 	plenty of room for future improvement.
> 
> [...]
> 
>  > Apart from calling exit() via G_fatal_error()[1], there are some
>  > other obstacles to making the GRASS libraries behave like normal
>  > libraries.  The most significant is the amount of static data which
>  > cannot be multiply instantiated, most of which is initialised at
>  > first use and often cannot be reset or otherwise modified thereafter.
> 
>  > The GRASS libraries are fundamentally designed to be used in
>  > "one-shot" modules. Errors are handled by printing a message then
>  > exiting. Variables are either initialised at startup from the .data
>  > and .bss segments or are initialised upon first use. Memory doesn't
>  > get explicitly de-allocated, as the operating system will do that
>  > when the module terminates.
> 
> 	Thanks for the clarification, but isn't the main question in
> 	whether there's any interest for it to be changed?

Oh, someone was to suddently turn up with a re-factored version where
all library functions behaved as desired and all modules had been
re-written accordingly, I'm sure that everyone would be happy.

[Assuming that you achieve this without significantly increasing the
job of writing modules. Minimising the effort involved in writing
modules takes precedence over most other considerations.]

The issue is whether there's any interest in expending the time and
effort involved in achieving this.

The idea is invariably proposed by someone with little experience of
GRASS programming, and (privately) laughed at by people with more
experience.

Once you've had a few goes at making a trivial interface change then
spending a few days finding and fixing everything which depended upon
the existing behaviour, you tend to become less eager to propose
radical changes.

>  > Making library functions suitable for use in persistent applications
>  > amounts to a complete re-write. If you're interested in taking that
>  > route, please let us know when you've finished.
> 
> 	To put it short, yes, I feel that the rewrite is both possible
> 	and in the long run, inevitable, and no, I'm not in the rush to
> 	start it just now.
> 
> 	An obvious approach to the problem may be to reimplement the
> 	non-returning functions on top of some (possibly newly
> 	introduced) functions that will return.
> 
> int
> do_foo_and_do_not_return (ARGUMENTS...)
> {
>   int rv = do_foo_and_return (ARGUMENTS...);
>   if (rv < 0) {
>     /* . */
>     G_fatal_error ("%s", CAUSE);
>   }
> 
>   /* . */
>   return rv;
> }

The main problem here is that you would need to re-write all of the
lower-level functions to return (which means that they need to perform
clean-up in the event of an error), with the non-returning versions
implemented as a thin veneer at the top level.

I'm assuming that you're willing to at least leave malloc() errors as
fatal, otherwise you will have to re-write almost everything. What
about low-level I/O errors (EIO, ENOSPC, EPERM etc)? If e.g. EIO while
reading $GISRC needs to be propagated up to the caller, any function
which ends up calling G_getenv() would need to be re-written, and
that's almost as big an issue as malloc() errors. Similar issues exist
for WIND, VAR and PROJ_INFO, and possibly for some map elements.

> 	It seems that there's both G_find_cell () and G_find_cell2 () to
> 	overcome a different, though in some respect close issue.
> 
> 	BTW, shouldn't G_find_cell () be marked as ``deprecated'' in
> 	`include/gisdefs.h' (if the compiler permits it), so that a
> 	warning will be issued for each its use?

There is a valid use for it, i.e. if you specifically want qualified
names to be split into map and mapset parts.

For 7.x, I would like to dispense with those functions altogether for
most modules. I.e. G_open_cell_old() etc would simply take the
(possibly qualified) name as an argument. Most modules wouldn't need
to know anything about mapsets; they would just pass the the value of
the ->answer field from the Option structure directly to the libgis
functions.

Most library functions would behave likewise, passing map names around
without needing to know the details. Only the lowest-level functions
which directly access files (e.g. G_{open,fopen}_*) would need to
decompose map names.

-- 
Glynn Clements <glynn at gclements.plus.com>


More information about the grass-dev mailing list