[GRASS5] r.sun bug

Hello
The latest version of r.sun contains the inline keyword in front of 3
function definitions in main.c . But as far as I can make out this is a
C++ keyword and doesn't work in C. It won't compile on IRIX anyway. I had
a look at this and thought seeing the aim is probably to make it run
faster it could use macros instead.

So the following patch seems to work and it still should run quickly. At
least it compiles again anyway. But I can't really test it. If there are
no objections I will add it to CVS in a day or two then.

Paul

Index: local_proto.h

RCS file: /grassrepository/grass/src/raster/r.sun/local_proto.h,v
retrieving revision 1.2
diff -u -r1.2 local_proto.h
--- local_proto.h 19 Feb 2003 12:36:51 -0000 1.2
+++ local_proto.h 21 Feb 2003 12:11:19 -0000
@@ -1,8 +1,6 @@
/* main.c */
int INPUT(void);
int OUTGR(void);
-double amax1(double, double);
-double amin1(double, double);
int min(int, int);
int max(int, int);
void com_par(void);
Index: main.c

RCS file: /grassrepository/grass/src/raster/r.sun/main.c,v
retrieving revision 1.8
diff -u -r1.8 main.c
--- main.c 19 Feb 2003 12:36:51 -0000 1.8
+++ main.c 21 Feb 2003 12:11:19 -0000
@@ -44,6 +44,10 @@
#define DSKY 1.0
#define DIST "1.0"

+#define AMAX1(arg1, arg2) ((arg1) >= (arg2) ? (arg1) : (arg2))
+#define AMIN1(arg1, arg2) ((arg1) <= (arg2) ? (arg1) : (arg2))
+#define DISTANCE2(x00, y00) ((xx0 - x00)*(xx0 - x00) + (yy0 - y00)*(yy0 - y00))
+
#include <stdio.h>
#include <stdlib.h>
#include <math.h>
@@ -83,8 +87,6 @@

int INPUT(void);
int OUTGR(void);
-double amax1(double, double);
-double amin1(double, double);
int min(int, int);
int max(int, int);
void com_par(void);
@@ -99,7 +101,6 @@
void line_x(int, int);
void line_y(int, int);
void cube(int, int);
-double distance2(double, double);
void (*func)(int, int);

void calculate(void);
@@ -133,36 +134,6 @@
double cbh, cdh;
double TOLER;

-inline double amax1(arg1,arg2)
- double arg1;
- double arg2;
-{
- double res;
- if (arg1>=arg2) {
- res = arg1;
- }
- else {
- res = arg2;
- }
- return res;
-}
-
-
-inline double amin1(arg1,arg2)
- double arg1;
- double arg2;
-{
- double res;
- if (arg1<=arg2) {
- res = arg1;
- }
- else {
- res = arg2;
- }
- return res;
-}
-
-
int
main(int argc, char *argv)
{
@@ -619,7 +590,7 @@
   {
     for (j = 0; j < n; j++)
     {
- zmax = amax1(zmax,z[i][j]);
+ zmax = AMAX1(zmax,z[i][j]);
     if ( o[i][j] != 0. ) {
        if( o[i][j] < 90. )
          o[i][j] = 90. - o[i][j];
@@ -921,10 +892,10 @@
       ypom = lum_Ly * lum_Ly;
       pom = sqrt(xpom + ypom);

- sr_min = amin1(sr_min,sunrise_time);
- sr_max = amax1(sr_max,sunrise_time);
- ss_min = amin1(ss_min,sunset_time);
- ss_max = amax1(ss_max,sunset_time);
+ sr_min = AMIN1(sr_min,sunrise_time);
+ sr_max = AMAX1(sr_max,sunrise_time);
+ ss_min = AMIN1(ss_min,sunset_time);
+ ss_max = AMAX1(ss_max,sunset_time);

       if (fabs(pom) > EPS) {
       A0 = lum_Ly / pom;
@@ -1207,7 +1178,7 @@
                 }

                 if (dist > 1.0)
- zp = amax1(c1,c2);
+ zp = AMAX1(c1,c2);
         }
         else
                   func = NULL;
@@ -1234,7 +1205,7 @@
     }

     if (dist > 1.0)
- zp = amax1(c1,c2);
+ zp = AMAX1(c1,c2);

         }
         else
@@ -1242,18 +1213,6 @@

}

-inline double distance2(x00, y00)
-double x00, y00;
-{
- double dx, dy;
-
- dx = xx0 - x00; dx *= dx;
- dy = yy0 - y00; dy *= dy;
-
-/* return (sqrt(dx + dy));*/
- return (dx + dy);
-}
-
void cube(jmin, imin)
int jmin, imin;
{
@@ -1268,14 +1227,14 @@
         y1 = (double)jmin * stepy;
     y2 = y1+stepy;

- v[0] = distance2(x1, y1);
+ v[0] = DISTANCE2(x1, y1);

     if(v[0]<vmin)
       {
       ig=0;
       vmin=v[0];
       }
- v[1] = distance2(x2, y1);
+ v[1] = DISTANCE2(x2, y1);

     if(v[1]<vmin)
       {
@@ -1283,14 +1242,14 @@
       vmin=v[1];
       }

- v[2] = distance2(x2, y2);
+ v[2] = DISTANCE2(x2, y2);
     if(v[2]<vmin)
       {
       ig=2;
       vmin=v[2];
       }

- v[3] = distance2(x1, y2);
+ v[3] = DISTANCE2(x1, y2);
     if(v[3]<vmin)
       {
       ig=3;
@@ -1314,7 +1273,7 @@
   if (dist > 1.0) {
     for (i = 0; i < 4; i++) {
     if (c[i] != UNDEFZ) {
- cmax = amax1(cmax,c[i]);
+ cmax = AMAX1(cmax,c[i]);
       zp = cmax;
       }
       else
@@ -1454,18 +1413,18 @@
          slope = s[j][i] * DEG;
       if(linkein!=NULL) {
         linke = li[j][i];
- li_max = amax1(li_max,linke);
- li_min = amin1(li_min,linke);
+ li_max = AMAX1(li_max,linke);
+ li_min = AMIN1(li_min,linke);
         }
       if (albedo != NULL) {
         alb = a[j][i];
- al_max = amax1(al_max,alb);
- al_min = amin1(al_min,alb);
+ al_max = AMAX1(al_max,alb);
+ al_min = AMIN1(al_min,alb);
         }
                         if (latin != NULL) {
       latitude = la[j][i];
- la_max = amax1(la_max,latitude);
- la_min = amin1(la_min,latitude);
+ la_max = AMAX1(la_max,latitude);
+ la_min = AMIN1(la_min,latitude);
             latitude = - latitude * DEG;
       }
   if (latin == NULL && lt == NULL) {
@@ -1498,14 +1457,14 @@
                   exit(0);
           }

- la_max = amax1(la_max,latitude);
- la_min = amin1(la_min,latitude);
+ la_max = AMAX1(la_max,latitude);
+ la_min = AMIN1(la_min,latitude);
                         latitude = - latitude * DEG;
        } else
     { /* ll projection */
       latitude = yp;
- la_max = amax1(la_max,latitude);
- la_min = amin1(la_min,latitude);
+ la_max = AMAX1(la_max,latitude);
+ la_min = AMIN1(la_min,latitude);
                         latitude = - latitude * DEG;
     }
   }

Paul Kelly wrote:

Hello
The latest version of r.sun contains the inline keyword in front of 3
function definitions in main.c . But as far as I can make out this is a
C++ keyword and doesn't work in C. It won't compile on IRIX anyway. I had
a look at this and thought seeing the aim is probably to make it run
faster it could use macros instead.

So the following patch seems to work and it still should run quickly. At
least it compiles again anyway. But I can't really test it. If there are
no objections I will add it to CVS in a day or two then.

Paul,

many thanks for the fix. I thinks it is a good solution. Please send me a copy of the file, I'll try to test it a little bit.
Thanks,

Jaro

Paul

Index: local_proto.h

RCS file: /grassrepository/grass/src/raster/r.sun/local_proto.h,v
retrieving revision 1.2
diff -u -r1.2 local_proto.h
--- local_proto.h 19 Feb 2003 12:36:51 -0000 1.2
+++ local_proto.h 21 Feb 2003 12:11:19 -0000
@@ -1,8 +1,6 @@
/* main.c */
int INPUT(void);
int OUTGR(void);
-double amax1(double, double);
-double amin1(double, double);
int min(int, int);
int max(int, int);
void com_par(void);
Index: main.c

RCS file: /grassrepository/grass/src/raster/r.sun/main.c,v
retrieving revision 1.8
diff -u -r1.8 main.c
--- main.c 19 Feb 2003 12:36:51 -0000 1.8
+++ main.c 21 Feb 2003 12:11:19 -0000
@@ -44,6 +44,10 @@
#define DSKY 1.0
#define DIST "1.0"

+#define AMAX1(arg1, arg2) ((arg1) >= (arg2) ? (arg1) : (arg2))
+#define AMIN1(arg1, arg2) ((arg1) <= (arg2) ? (arg1) : (arg2))
+#define DISTANCE2(x00, y00) ((xx0 - x00)*(xx0 - x00) + (yy0 - y00)*(yy0 - y00))
+
#include <stdio.h>
#include <stdlib.h>
#include <math.h>
@@ -83,8 +87,6 @@

int INPUT(void);
int OUTGR(void);
-double amax1(double, double);
-double amin1(double, double);
int min(int, int);
int max(int, int);
void com_par(void);
@@ -99,7 +101,6 @@
void line_x(int, int);
void line_y(int, int);
void cube(int, int);
-double distance2(double, double);
void (*func)(int, int);

void calculate(void);
@@ -133,36 +134,6 @@
double cbh, cdh;
double TOLER;

-inline double amax1(arg1,arg2)
- double arg1;
- double arg2;
-{
- double res;
- if (arg1>=arg2) {
- res = arg1;
- }
- else {
- res = arg2;
- }
- return res;
-}
-
-inline double amin1(arg1,arg2)
- double arg1;
- double arg2;
-{
- double res;
- if (arg1<=arg2) {
- res = arg1;
- }
- else {
- res = arg2;
- }
- return res;
-}
-
int
main(int argc, char *argv)
{
@@ -619,7 +590,7 @@
  {
    for (j = 0; j < n; j++)
    {
- zmax = amax1(zmax,z[i][j]);
+ zmax = AMAX1(zmax,z[i][j]);
    if ( o[i][j] != 0. ) {
       if( o[i][j] < 90. )
         o[i][j] = 90. - o[i][j];
@@ -921,10 +892,10 @@
      ypom = lum_Ly * lum_Ly;
      pom = sqrt(xpom + ypom);

- sr_min = amin1(sr_min,sunrise_time);
- sr_max = amax1(sr_max,sunrise_time);
- ss_min = amin1(ss_min,sunset_time);
- ss_max = amax1(ss_max,sunset_time);
+ sr_min = AMIN1(sr_min,sunrise_time);
+ sr_max = AMAX1(sr_max,sunrise_time);
+ ss_min = AMIN1(ss_min,sunset_time);
+ ss_max = AMAX1(ss_max,sunset_time);

      if (fabs(pom) > EPS) {
      A0 = lum_Ly / pom;
@@ -1207,7 +1178,7 @@
                }

                if (dist > 1.0)
- zp = amax1(c1,c2);
+ zp = AMAX1(c1,c2);
        }
        else
                  func = NULL;
@@ -1234,7 +1205,7 @@
    }

    if (dist > 1.0)
- zp = amax1(c1,c2);
+ zp = AMAX1(c1,c2);

        }
        else
@@ -1242,18 +1213,6 @@

}

-inline double distance2(x00, y00)
-double x00, y00;
-{
- double dx, dy;
-
- dx = xx0 - x00; dx *= dx;
- dy = yy0 - y00; dy *= dy;
-
-/* return (sqrt(dx + dy));*/
- return (dx + dy);
-}
-
void cube(jmin, imin)
int jmin, imin;
{
@@ -1268,14 +1227,14 @@
        y1 = (double)jmin * stepy;
    y2 = y1+stepy;

- v[0] = distance2(x1, y1);
+ v[0] = DISTANCE2(x1, y1);

    if(v[0]<vmin)
      {
      ig=0;
      vmin=v[0];
      }
- v[1] = distance2(x2, y1);
+ v[1] = DISTANCE2(x2, y1);

    if(v[1]<vmin)
      {
@@ -1283,14 +1242,14 @@
      vmin=v[1];
      }

- v[2] = distance2(x2, y2);
+ v[2] = DISTANCE2(x2, y2);
    if(v[2]<vmin)
      {
      ig=2;
      vmin=v[2];
      }

- v[3] = distance2(x1, y2);
+ v[3] = DISTANCE2(x1, y2);
    if(v[3]<vmin)
      {
      ig=3;
@@ -1314,7 +1273,7 @@
  if (dist > 1.0) {
    for (i = 0; i < 4; i++) {
    if (c[i] != UNDEFZ) {
- cmax = amax1(cmax,c[i]);
+ cmax = AMAX1(cmax,c[i]);
      zp = cmax;
      }
      else
@@ -1454,18 +1413,18 @@
         slope = s[j][i] * DEG;
      if(linkein!=NULL) {
        linke = li[j][i];
- li_max = amax1(li_max,linke);
- li_min = amin1(li_min,linke);
+ li_max = AMAX1(li_max,linke);
+ li_min = AMIN1(li_min,linke);
        }
      if (albedo != NULL) {
        alb = a[j][i];
- al_max = amax1(al_max,alb);
- al_min = amin1(al_min,alb);
+ al_max = AMAX1(al_max,alb);
+ al_min = AMIN1(al_min,alb);
        }
                        if (latin != NULL) {
      latitude = la[j][i];
- la_max = amax1(la_max,latitude);
- la_min = amin1(la_min,latitude);
+ la_max = AMAX1(la_max,latitude);
+ la_min = AMIN1(la_min,latitude);
            latitude = - latitude * DEG;
      }
  if (latin == NULL && lt == NULL) {
@@ -1498,14 +1457,14 @@
                  exit(0);
          }

- la_max = amax1(la_max,latitude);
- la_min = amin1(la_min,latitude);
+ la_max = AMAX1(la_max,latitude);
+ la_min = AMIN1(la_min,latitude);
                        latitude = - latitude * DEG;
       } else
    { /* ll projection */
      latitude = yp;
- la_max = amax1(la_max,latitude);
- la_min = amin1(la_min,latitude);
+ la_max = AMAX1(la_max,latitude);
+ la_min = AMIN1(la_min,latitude);
                        latitude = - latitude * DEG;
    }
  }

_______________________________________________
grass5 mailing list
grass5@grass.itc.it
http://grass.itc.it/mailman/listinfo/grass5

Paul Kelly wrote:

The latest version of r.sun contains the inline keyword in front of 3
function definitions in main.c . But as far as I can make out this is a
C++ keyword and doesn't work in C. It won't compile on IRIX anyway.

It works in gcc. Everyone uses gcc, right? :wink:

Seriously, I'm glad that someone is testing this stuff on something
other than Linux/x86. In many respects, Linux/x86 is the worst
possible test platform, as so many things which work there won't work
anywhere else. E.g.

1. Alignment restrictions: unaligned memory access (e.g. reading a
32-bit word from an odd address) works on x86 but fails (either raises
a signal or returns bogus data) on most other architectures.

2. gcc extensions: gcc has many extensions (e.g. "inline", C++ "//"
comments, recently C9X), and many of them are enabled by default.

3. GNU libc: while some platforms have non-core functionality
(sockets, name-service, I18N) in separate libraries, GNU libc puts it
into libc itself. Consequently, the tests get omitted from configure
and the definitions get omitted from the Gmakefiles.

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

On Fri, Feb 21, 2003 at 02:28:05PM +0000, Glynn Clements wrote:

Paul Kelly wrote:

> The latest version of r.sun contains the inline keyword in front of 3
> function definitions in main.c . But as far as I can make out this is a
> C++ keyword and doesn't work in C. It won't compile on IRIX anyway.

It works in gcc. Everyone uses gcc, right? :wink:

2. gcc extensions: gcc has many extensions (e.g. "inline", C++ "//"
comments, recently C9X), and many of them are enabled by default.

Could add "-ansi" or "-std=iso9899:199409" (include 1994 amendments)
to the CFLAGS for GCC. The "-ansi" flag disables "inline" and "//",
but possibly also a few builtins that we'd want like "bzero" and "index"
(though, those aren't really needed).

--
echo ">gra.fcw@2ztr< eryyvZ .T pveR" | rot13 | reverse

Eric G. Miller wrote:

> > The latest version of r.sun contains the inline keyword in front of 3
> > function definitions in main.c . But as far as I can make out this is a
> > C++ keyword and doesn't work in C. It won't compile on IRIX anyway.
>
> It works in gcc. Everyone uses gcc, right? :wink:

> 2. gcc extensions: gcc has many extensions (e.g. "inline", C++ "//"
> comments, recently C9X), and many of them are enabled by default.

Could add "-ansi" or "-std=iso9899:199409" (include 1994 amendments)
to the CFLAGS for GCC. The "-ansi" flag disables "inline" and "//",
but possibly also a few builtins that we'd want like "bzero" and "index"
(though, those aren't really needed).

Unfortunately, "-ansi" also disables all of the feature test macros,
so you have to re-enable them manually, e.g.

CFLAGS='-g -ansi -D_BSD_SOURCE=1 -D_SVID_SOURCE=1 -D_POSIX_SOURCE=1 -D_POSIX_C_SOURCE=199506L'

_BSD_SOURCE and _SVID_SOURCE are sufficient for everything except
sigaction(), which also requires _POSIX_SOURCE.

However, using _GNU_SOURCE doesn't work, as sys/ucontext.h (included
by signal.h) and curses.h both try to define ERR.

The above CFLAGS setting works, and highlighted the "inline" error in
src/raster/r.sun/main.c, as well as the use of C++ comments in
src/libes/dbmi/drivers/odbc/globals.h.

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

On Mon, Feb 24, 2003 at 06:53:25PM +0000, Glynn Clements wrote:
[...]

Unfortunately, "-ansi" also disables all of the feature test macros,
so you have to re-enable them manually, e.g.

CFLAGS='-g -ansi -D_BSD_SOURCE=1 -D_SVID_SOURCE=1 -D_POSIX_SOURCE=1 -D_POSIX_C_SOURCE=199506L'

_BSD_SOURCE and _SVID_SOURCE are sufficient for everything except
sigaction(), which also requires _POSIX_SOURCE.

However, using _GNU_SOURCE doesn't work, as sys/ucontext.h (included
by signal.h) and curses.h both try to define ERR.

The above CFLAGS setting works, and highlighted the "inline" error in
src/raster/r.sun/main.c, as well as the use of C++ comments in
src/libes/dbmi/drivers/odbc/globals.h.

Are there objections to hardcode above CFLAGS in 5.1 to
force pretty programming?

Markus

Hello Markus

On Wed, 26 Feb 2003, Markus Neteler wrote:

> CFLAGS='-g -ansi -D_BSD_SOURCE=1 -D_SVID_SOURCE=1 -D_POSIX_SOURCE=1 -D_POSIX_C_SOURCE=199506L'
>
> The above CFLAGS setting works, and highlighted the "inline" error in
> src/raster/r.sun/main.c, as well as the use of C++ comments in
> src/libes/dbmi/drivers/odbc/globals.h.

Are there objections to hardcode above CFLAGS in 5.1 to
force pretty programming?

That is a nice idea but are they not gcc-specific flags? There is already
a hard-coded -Wall in the GRASS 5.1 cflags which causes problems for non-gcc
compilers (for information it should be -fullwarn for the IRIX cc). It probably
shouldn't be too hard to make their inclusion specific to if gcc is detected;
(just I don't know how to do it.)

Paul

Paul Kelly wrote:

> > CFLAGS='-g -ansi -D_BSD_SOURCE=1 -D_SVID_SOURCE=1 -D_POSIX_SOURCE=1 -D_POSIX_C_SOURCE=199506L'
> >
> > The above CFLAGS setting works, and highlighted the "inline" error in
> > src/raster/r.sun/main.c, as well as the use of C++ comments in
> > src/libes/dbmi/drivers/odbc/globals.h.
>
> Are there objections to hardcode above CFLAGS in 5.1 to
> force pretty programming?

That is a nice idea but are they not gcc-specific flags? There is already
a hard-coded -Wall in the GRASS 5.1 cflags which causes problems for non-gcc
compilers (for information it should be -fullwarn for the IRIX cc). It probably
shouldn't be too hard to make their inclusion specific to if gcc is detected;
(just I don't know how to do it.)

The AC_PROG_CC autoconf macro sets the variable GCC to "yes" if the
C compiler is gcc.

However, the set of flags has to be compatible with any third-party
headers which GRASS uses, as well as GRASS itself. So, I recommend
that we don't add any unnecessary switches to CFLAGS.

Individual users can always force the use of specific flags if they
wish. So long as someone compiles the code with "-ansi ..." regularly,
most gcc-isms should be caught.

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