[GRASS-dev] is this compiler-safe?

[Glynn Clements]

Markus Metz wrote:

> I found two code snippets in the segment library (all branches) that 
> look fishy to me:
> 
> the first is in relbr6 here
> https://trac.osgeo.org/grass/browser/grass/branches/releasebranch_6_4/lib/segment/format.c#L135
> 
> should
>     if (lseek(fd, 0L, SEEK_SET) == (off_t) - 1) {
> 
> not be
>     if (lseek(fd, 0L, SEEK_SET) == (off_t) -1) {

The two are equivalent; the whitespace isn't significant here.

> because I can't see where 1 is subtracted from? Introduced with 
> automated indentation in r32527

There is no subtraction; the "-" is treated as a unary minus
(negation) operator in this context. If you were to change it to
something which cannot be interpreted as a unary operator
(e.g. / or %), you would get an error.

> the second is in relbr6 here:
> https://trac.osgeo.org/grass/browser/grass/branches/releasebranch_6_4/lib/segment/format.c#L178
> 
>     if ((bytes_wrote = write(fd, &x, sizeof(int)) == sizeof(int)) < 0)

Ouch. There's no way that that can make sense. As it stands, it's
parsed as:

      if ((bytes_wrote = (write(fd, &x, sizeof(int)) == sizeof(int))) < 0)

i.e. bytes_wrote will be either 0 or 1, so the "... < 0" will always
be false.

> I am missing parentheses somewhere and would rather use
> 
>     if ((bytes_wrote = write(fd, &x, sizeof(int))) != sizeof(int))
>     G_warning("%s", strerror(errno));
> 
>     return (bytes_wrote == sizeof(int));

I would use:

	static int write_int(int fd, int n)
	{
	    if (write(fd, &n, sizeof(int)) != sizeof(int)) {
		G_warning("%s", strerror(errno));
		return 0;
	    }
	
	    return 1;
	}

There's no need to make a copy of "n"; it's already a copy of the
actual argument.

Actually, looking at the code in context, I'd write:

	static void write_int(int fd, int n)
	{
	    if (write(fd, &n, sizeof(int)) != sizeof(int))
		G_fatal_error("%s", strerror(errno));
	}

Lets face it: you aren't going to be recovering from this situation,
are you? If _segment_format fails, the module fails.

While you're there, can you fix this nonsense:

	if (file_size > INT_MAX) {
?

OTOH, the segment library is long overdue for being replaced with
something more efficient.

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