[GRASS5] [bug #3742] (grass) r.out.bin output incorrect compared with r.out.netcdf

[Glynn Clements]

Hamish wrote:

> > > > Subject: r.out.bin output incorrect compared with r.out.netcdf
> ..
> > > > > []% r.out.bin -h null=NaN in=srtm out=srtm.grd
> > > 
> > > note null=NaN should not work [yet it does! G_parser bug!].
> > 
> > G_parser() doesn't actually check that argument values are legitimate
> > unless the ->options field is non-null.
> 
> I didn't know that. Ok, it's just a string.
> 
> Should it be testing the string against opt->type? (,opt->multiple)

Arguably.

> (ie is there a reason not to do so, or has it just not been done yet?)

Given that it hasn't been done so far, it's always possible that some
modules are setting opt->type to TYPE_INTEGER but allowing arguments
which aren't actually integers. Adding the type-checking would prevent
such "enhancements" from working.

I'd say: go for it. If it breaks a module, fix the module (i.e. change
it to use TYPE_STRING instead).

> Glynn, I was hoping you would know the answer to the outstanding GMT 
> header code byteswap question?
> 
>     /* Set up Parameters for GMT header */
>     if (flag.gmt_hd->answer) {
> ...
> 	if (flag.swap->answer)
> 	    TIFFSwabLong((uint32 *) & null_str);
> 	sprintf(buf, "%d", null_str);
> ...
>     }
> 
> does this pollute null_str byteswapping later on in the code?
>  (ie swapping an already swapped variable)

It looks that way.

Also, this:

	fprintf(fp_1, "nodata %f\n", null_str);

is bogus, as null_str is an int, but fprintf will be expecting a
double.

I suggest that null_str should actually be a double, and the
conversion of the argument to null= should have separate integer/FP
cases depending upon the input map type.

> also should that (and the rest of the code) test be
> if (swapFlag)
>  not
> if (flag.swap->answer)

No.

> or should "swapFlag" be renamed something better?

Yes. It should be called something like outputLittleEndian or
outputIntel, as that's what it represents.

On a related note, the meaning of the -s flag is ill-conceived; the
output shouldn't depend upon the host's byte-order, only upon the
options.

It would be better to have e.g. endian={big,little,host}, so you can
use endian=big or endian=little to get consistent results regardless
of the byte order of the host on which you happen to be executing the
command, or endian=host if you just want whatever will be most
efficient on the system you're using.

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