[GRASS-dev] Use of "const", qualified map names

GRASS GIS GRASS GIS developer
1

Glynn Clements wrote:

Which reminds me of another outstanding clean-up task: library
functions should use "const" where appropriate.

As it stands, functions which take a non-const pointer but which don't
actually modify the value are so common that a lot of application code
tends to just assume that the missing "const" is an oversight and that
the value won't actually be modified.

Then it gets bitten by when the function really does modify the value
(G_find_cell() is a common culprit, as it strips off any @mapset
suffix from the map name).

We need to get into the habit of using "const" wherever possible, so
that the lack of it indicates that the string (or other value) really
will be modified (rather than simply indicating that someone forgot
the "const").

This needs to happen from the bottom up, as once a function declares a
parameter as "const", it can't pass it to a function which requires a
non-const parameter (using a type cast can hide a real problem, in the
case where the function really does modify the argument).

Over the last two days I have gone through libgis and added "const"
qualifiers wherever they were appropriate (as well as a few other
minor clean-ups).

However, this required a subtle API change.

Previously, just about every libgis function which accepted parameters
of the form (char *name, char *mapset, ...) would end up normalising
the name (removing any "@mapset" part). In most cases, this was likely
to be accidental: an artifact of using G_find_file() etc.

Apart from meaning that you couldn't pass a "const char *" for the
name, it could cause problems if you weren't expecting it (certainly,
I've been bitten by it at least once).

Following my changes, the only functions which should strip the
@mapset part from a map name are the G_find_{file,cell,vector}
functions (which return the mapset where the file was found). Anything
else should leave the name intact (but will still honour any @mapset
part).

There is a remote possibility that some module somewhere was actually
relying upon the existing behaviour (rather than using G_find_*() to
explicitly normalise the name). As this would be almost impossible to
track down methodically, all that I can suggest is to be on the
lookout for modules misbehaving when given qualified map names.

On a related note, I've modified the Tk map browser
(lib/gtcltk/select.tcl) to always include the @mapset part in map
names. Previously, it omitted it if the mapset was the current mapset,
which will fail if you have other mapsets ahead of the current mapset
in the search path.

[Note: you can't make this check at the point that the map was
selected, as the mapset path might be modified afterwards. Then,
redisplay would suddenly show the wrong map.]

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

2

Glynn Clements wrote:

Over the last two days I have gone through libgis and added "const"
qualifiers wherever they were appropriate (as well as a few other
minor clean-ups).

However, this required a subtle API change.

Previously, just about every libgis function which accepted parameters
of the form (char *name, char *mapset, ...) would end up normalising
the name (removing any "@mapset" part). In most cases, this was likely
to be accidental: an artifact of using G_find_file() etc.

Apart from meaning that you couldn't pass a "const char *" for the
name, it could cause problems if you weren't expecting it (certainly,
I've been bitten by it at least once).

Following my changes, the only functions which should strip the
@mapset part from a map name are the G_find_{file,cell,vector}
functions (which return the mapset where the file was found). Anything
else should leave the name intact (but will still honour any @mapset
part).

There is a remote possibility that some module somewhere was actually
relying upon the existing behaviour (rather than using G_find_*() to
explicitly normalise the name). As this would be almost impossible to
track down methodically, all that I can suggest is to be on the
lookout for modules misbehaving when given qualified map names.

Soeren found one major instance: G__check_fp_type(), used by
G__open_cell_old() (and thus everything which uses it).

This caused a significant number of modules to fail when reading FP
maps.

I've fixed that particular case in CVS, but there are others. Mostly,
they relate to code which accesses the cell_misc directory.

[Aside: whoever came up with the cell_misc structure needs major
therapy. Getting rid of it needs to be the #1 priority for 7.x.]

The reason is that all of the G_open_* functions which take
element/name (and sometimes mapset) have to be "abused" in order to
work with cell_misc, by passing cell_misc/<mapname> for the "element"
parameter and <element> for the "name" parameter.

Because the name isn't actually being passed as the name, but is
embedded in the element parameter, the code which strips the @mapset
part never gets applied. When using G_open_* correctly (i.e. passing
the name as the "name" parameter), this isn't an issue, as the name
will be normalised (split it into an unqualified name and a separate
mapset) automatically.

This doesn't affect all modules: only those which fail to normalise
the map name are affected.

This has only become an issue with the recent change because,
previously, almost any libgis function which accepted a map/mapset
pair would end up normalising the name as a side-effect.

Essentially, modules which use G_find_cell2() instead of G_find_cell()
are affected. AFAICT, modules appear to be almost evenly divided
between the use of G_find_cell() and G_find_cell2().

Now, a question:

Should I leave these modules alone, in order to identify other
functions which don't like being passed qualified map names, or should
I just fix the modules?

The former is better in the long run[1], but means that, in the short
term, we will probably keep bumping into cases where qualified map
names fail.

[1] Ultimately, I hope to remove all knowledge of mapsets from the
modules. Modules should just be able to pass opt->answer directly to
libgis functions without having to deal with G_find_cell(), mapset
parameters and the like.

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