[GRASS5] g.region G_usage() commented out?

Russell_Nelson · May 14, 2002, 4:11pm

The inter version of g.region has a call to G_usage() in die()
commented out. Anybody know why this might be? Telling somebody that
something is an "** illegal value **" is almost completely useless to
them. Much better to explain what change is necessary to bring the
value within bounds.

Glynn_Clements · May 14, 2002, 5:12pm

Russell Nelson wrote:

The inter version of g.region has a call to G_usage() in die()
commented out.

die() is in the "cmd" version of g.region, not the "inter" version.

Anybody know why this might be?

There is seldom any point in calling G_usage() directly. G_usage()
generates a usage message based upon the options and flags which have
been defined by G_define_option() and G_define_flag() respectively.

G_parser() will call G_usage() if it detects an error (a required
option wasn't specified, an unknown option was specified, etc), or if
the "help" option was given. If G_parser() indicates success, the
nature of any remaining errors is such that G_usage() is unlikely to
provide any meaningful clues.

Telling somebody that
something is an "** illegal value **" is almost completely useless to
them. Much better to explain what change is necessary to bring the
value within bounds.

If die() is called, the error is that one of the values isn't in a
suitable format; G_usage() won't help here. The set of supported
variations is sufficiently large that it probably isn't sensible to
document them within the usage message.

However, the manual page should mention the fact that, for lat/lon
locations, values have to be in DMS. But it doesn't.

--
Glynn Clements <glynn.clements@virgin.net>

Russell_Nelson · May 15, 2002, 4:38am

Glynn Clements writes:
> Russell Nelson wrote:
>
> > The inter version of g.region has a call to G_usage() in die()
> > commented out.
>
> die() is in the "cmd" version of g.region, not the "inter" version.

Quite right, yes I meant the cmd version.

> > Telling somebody that
> > something is an "** illegal value **" is almost completely useless to
> > them. Much better to explain what change is necessary to bring the
> > value within bounds.
>
> If die() is called, the error is that one of the values isn't in a
> suitable format; G_usage() won't help here.

What about changing die() so that it also prints the ->description
string? Like the diff below? It's not perfect, but it's an
improvement.

diff -u -r1.8 main.c
--- src/general/g.region/cmd/main.c 22 Jan 2002 04:51:04 -0000 1.8
+++ src/general/g.region/cmd/main.c 15 May 2002 04:34:35 -0000
@@ -704,7 +704,7 @@

static void die(struct Option *parm)
{
- fprintf(stderr,"<%s=%s> ** illegal value **\n\n", parm->key, parm->answer);
+ fprintf(stderr,"<%s=%s> not recognized, expecting:\n%s\n", parm->key, parm->answer, parm->description);
        /*
     G_usage();
     */

Eric_G_Miller · May 15, 2002, 6:15am

On Wed, May 15, 2002 at 12:38:18AM -0400, Russell Nelson wrote:
[snip]

What about changing die() so that it also prints the ->description
string? Like the diff below? It's not perfect, but it's an
improvement.

diff -u -r1.8 main.c
--- src/general/g.region/cmd/main.c 22 Jan 2002 04:51:04 -0000 1.8
+++ src/general/g.region/cmd/main.c 15 May 2002 04:34:35 -0000
@@ -704,7 +704,7 @@

static void die(struct Option *parm)
{
- fprintf(stderr,"<%s=%s> ** illegal value **\n\n", parm->key, parm->answer);
+ fprintf(stderr,"<%s=%s> not recognized, expecting:\n%s\n", parm->key, parm->answer, parm->description);
        /*
     G_usage();
     */

I'm not sure that adds anything to the error report.

On a general note, would it be desirable to have G_{lat|lon|llres}_scan
fall back to reading doubles and then range check the value?

Nitpick: I dislike "illegal value". IMHO, "invalid input" is more
"correct" since the law has nothing to do with the matter. Also, no
good reason not to use G_fatal_error() here. Technically, exit(1) is
not well defined either (exit(EXIT_FAILURE) is).

--
Eric G. Miller <egm2@jps.net>

Glynn_Clements · May 15, 2002, 9:45am

Eric G. Miller wrote:

> What about changing die() so that it also prints the ->description
> string? Like the diff below? It's not perfect, but it's an
> improvement.
>
> diff -u -r1.8 main.c
> --- src/general/g.region/cmd/main.c 22 Jan 2002 04:51:04 -0000 1.8
> +++ src/general/g.region/cmd/main.c 15 May 2002 04:34:35 -0000
> @@ -704,7 +704,7 @@
>
> static void die(struct Option *parm)
> {
> - fprintf(stderr,"<%s=%s> ** illegal value **\n\n", parm->key, parm->answer);
> + fprintf(stderr,"<%s=%s> not recognized, expecting:\n%s\n", parm->key, parm->answer, parm->description);
> /*
> G_usage();
> */

I'm not sure that adds anything to the error report.

Well, the description field will indicate that the format has to be
dd:mm:ss for lat/lon locations.

However, the mechanism which is used involves calling library
functions before G_parser() has returned, which would stop working if
we were implement "standard" options such as "location=".

On a general note, would it be desirable to have G_{lat|lon|llres}_scan
fall back to reading doubles and then range check the value?

I would say so.

Nitpick: I dislike "illegal value". IMHO, "invalid input" is more
"correct" since the law has nothing to do with the matter. Also, no
good reason not to use G_fatal_error() here. Technically, exit(1) is
not well defined either (exit(EXIT_FAILURE) is).

At least exit(1) is an improvement over exit(-1); the latter is all
too common. Also, while exit(1) may not be defined by ANSI C, the
various Unix specifications all dictate that zero indicates success
and non-zero indicates failure.

--
Glynn Clements <glynn.clements@virgin.net>

Russell_Nelson · May 15, 2002, 2:41pm

Glynn Clements writes:
> > I'm not sure that adds anything to the error report.
>
> Well, the description field will indicate that the format has to be
> dd:mm:ss for lat/lon locations.

I can't imagine it hurts to give a user more information.

> However, the mechanism which is used involves calling library
> functions before G_parser() has returned, which would stop working if
> we were implement "standard" options such as "location=".

So, you're saying that the entire die() function is not wisely implemented?

> > On a general note, would it be desirable to have G_{lat|lon|llres}_scan
> > fall back to reading doubles and then range check the value?
>
> I would say so.

Speaking of which, what's this MARKER stuff in those functions? Is
that a holder from the FORTRAN implementation? Anybody mind if I add
code to read doubles and delete the MARKER stuff at the same time?

Glynn_Clements · May 15, 2002, 3:17pm

Russell Nelson wrote:

> However, the mechanism which is used involves calling library
> functions before G_parser() has returned, which would stop working if
> we were implement "standard" options such as "location=".

So, you're saying that the entire die() function is not wisely implemented?

No, I'm saying that the way in which the "description" fields are
initialised isn't a good idea.

Basically, I'm opposed to main() calling anything other than a few
specific functions[1] before G_parser() has returned.

The main reason is to allow the parser to support some "built in"
options (e.g. location=, mapset=, monitor=), in the same way that all
Xt-based programs support -display, -bg, -fg, -font etc.

A secondary reason is that, when the "help" or "--interface-description"
switches are used, the output depends upon the context (current
location, mapset, region, monitor etc), but with no indication that this
is the case.

[1] G_gisinit, G_define_module, G_define_{option,flag}.

--
Glynn Clements <glynn.clements@virgin.net>

Russell_Nelson · May 15, 2002, 3:31pm

Glynn Clements writes:
> Russell Nelson wrote:
> > So, you're saying that the entire die() function is not wisely implemented?
>
> No, I'm saying that the way in which the "description" fields are
> initialised isn't a good idea.

Ahhhh, I see. Okay, then I'll leave that alone and add double
scanning/bounds-checking to G_{lat,lon,llres}_scan, and remove the
MARKER code (if it is indeed silly).

Glynn_Clements · May 17, 2002, 12:10pm

Russell Nelson wrote:

> > On a general note, would it be desirable to have G_{lat|lon|llres}_scan
> > fall back to reading doubles and then range check the value?
>
> I would say so.

Doh! Actually, no, it wouldn't be desirable. Why? Because the only
place those functions are called is from G_scan_{northing,easting,resolution},
and if those functions fail, then they fall back to simply scanning a double.

But while I was looking at the code, I spotted two ways to simplify
the code. Patch below:

Are you sure about the removal of the "marker" code? I would guess
that it's there for a reason (although I have no idea what that reason
might be).

--
Glynn Clements <glynn.clements@virgin.net>

Russell_Nelson · May 20, 2002, 2:48am

Glynn Clements writes:
> Are you sure about the removal of the "marker" code? I would guess
> that it's there for a reason (although I have no idea what that reason
> might be).

Reasonably. I tracked down all the subroutines called to make sure
they are happy with a null string. Perhaps it's something left over
from the Fortran code? I seem to recall needing to put such things
into Fortran prorgams.

Glynn_Clements · May 20, 2002, 1:06pm

Russell Nelson wrote:

> Are you sure about the removal of the "marker" code? I would guess
> that it's there for a reason (although I have no idea what that reason
> might be).

Reasonably. I tracked down all the subroutines called to make sure
they are happy with a null string. Perhaps it's something left over
from the Fortran code? I seem to recall needing to put such things
into Fortran prorgams.

I'm not sure that this has anything to do with Fortran.

AFAICT, the code appears to be ensuring that the code processes the
entire string, and not just some initial portion of it.

--
Glynn Clements <glynn.clements@virgin.net>

Russell_Nelson · May 20, 2002, 1:50pm

Glynn Clements writes:
> AFAICT, the code appears to be ensuring that the code processes the
> entire string, and not just some initial portion of it.

But tbuf is filled by copying characters from buf up to the first
null. Therefore, buf cannot contain any embedded nulls, and
strlen(buf) is going to be equal to strchr(tbuf, MARKER) - tbuf every
time. So why not just run up to the null? The only reason I can see
to have MARKER would be if you wanted to safely look at buf[i] and
buf[i+1] without first checking to see if buf[i] is a '\0'. The
original code didn't do that, but ahhhh, I see that I have introduced
a potential segfault. Revised patch, which actually is simpler and
easier to understand:

Index: src/libes/gis/ll_scan.c

RCS file: /home/grass/grassrepository/grass/src/libes/gis/ll_scan.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 ll_scan.c
--- src/libes/gis/ll_scan.c 29 Dec 1999 15:10:30 -0000 1.1.1.1
+++ src/libes/gis/ll_scan.c 20 May 2002 13:49:13 -0000
@@ -53,13 +53,9 @@

int G_llres_scan ( char *buf, double *res)
{
- char tbuf[100];
-
- sprintf (tbuf, "%se", buf);
- return scan_ll (tbuf, "we", res, 0);
+ return scan_ll (buf, NULL, res, 0);
}

-#define MARKER 1
static int scan_ll (
     char *buf,
     char *dir,
@@ -70,10 +66,6 @@
     int d,m,s;
     char ps[20], *pps;
     double p, f;
- char tbuf[100];
-
- sprintf (tbuf, "%s%c", buf, MARKER); /* add a marker at end of string */
- buf = tbuf;

if (sscanf (buf, "%d:%d:%d.%[0123456789]%[^\n]", &d, &m, &s, ps, h) == 5)
{
@@ -117,13 +109,16 @@

G_strip (h);

- if (*result == 0.0 && *h == MARKER)
+ if (*result == 0.0 && *h == '\0')
return (1);

+ if (dir == NULL)
+ return *h == '\0';
+
     if (*h >= 'A' && *h <= 'Z') *h += 'a' - 'A';
     if (*h != dir[0] && *h != dir[1])
   return 0;
- if (h[1] != MARKER)
+ if (h[1] != '\0')
   return 0;

if (*h == dir[0] && *result != 0.0)

Glynn_Clements · May 20, 2002, 2:56pm

Russell Nelson wrote:

> AFAICT, the code appears to be ensuring that the code processes the
> entire string, and not just some initial portion of it.

But tbuf is filled by copying characters from buf up to the first
null. Therefore, buf cannot contain any embedded nulls, and
strlen(buf) is going to be equal to strchr(tbuf, MARKER) - tbuf every
time. So why not just run up to the null? The only reason I can see
to have MARKER would be if you wanted to safely look at buf[i] and
buf[i+1] without first checking to see if buf[i] is a '\0'. The
original code didn't do that, but ahhhh, I see that I have introduced
a potential segfault. Revised patch, which actually is simpler and
easier to understand:

It still changes the semantics of the function. The existing version
will reject strings which contain trailing whitespace, due to:

if (h[1] != MARKER)
return 0;

If the marker is removed, G_strip() will strip any trailing whitespace
before the test is performed.

I'm not suggesting that I have a personal preference for the existing
behaviour; I'm just asking you (or, for that matter, anyone else)
whether you are sure that it's safe to change it.

My *suspicion* is that it *probably* is safe.

--
Glynn Clements <glynn.clements@virgin.net>

Russell_Nelson · May 20, 2002, 9:19pm

Glynn Clements writes:
> It still changes the semantics of the function. The existing version
> will reject strings which contain trailing whitespace, due to:
>
> if (h[1] != MARKER)
> return 0;
>
> If the marker is removed, G_strip() will strip any trailing whitespace
> before the test is performed.

True enough. Is this behavior anybody wishes to preserve?