[GRASS-dev] is this compiler-safe?

[Paul Kelly]

On Wed, 10 Feb 2010, 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) {
>
> because I can't see where 1 is subtracted from? Introduced with automated 
> indentation in r32527

Hello Markus,
Well, looks like a bug in the indent program that got confused by the cast 
and thought the - was being used as a binary rather than unary operator. 
But is it not only an aesthetic problem? As far as I can see the code 
should do exactly the same thing?

> 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)
>
> 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));
>
> just to be on the safe side

Yes I suppose you saw too that the code that calls this function assumes 
if the return value is non-zero that it succeeded, which doesn't seem 
right. I would suggest to make it even clearer by removing the redundant 
variable x, correcting the grammar (written instead of wrote) and 
rearranging:

Index: format.c
===================================================================
--- format.c    (revision 40905)
+++ format.c    (working copy)
@@ -170,15 +170,12 @@

  static int write_int(int fd, int n)
  {
-    int x;
-    int bytes_wrote;
+    int bytes_written = write(fd, &n, sizeof(int));

-    x = n;
-
-    if ((bytes_wrote = write(fd, &x, sizeof(int)) == sizeof(int)) < 0)
+    if (bytes_written != sizeof(int))
         G_warning("%s", strerror(errno));

-    return bytes_wrote;
+    return (bytes_written == sizeof(int));
  }

I think that is yet easier to read - do you agree?

Paul