Re: XWisp2 with gcc patch

Hi!

El Wed, Sep 14, 2005 at 11:00:05AM +0200, Rob Hamerling escribio:
> 
> I have read your patches for xwisp2, and have some comments/questions.
> Please have some patience with me, I have hardly any experience with 
> using Linux and even less with programming for Linux...

No problems, thanks for doing this work!

> Daniel Serpell wrote:
> 
> >xwisp2.170.linux.unwarn.patch :
> >        This fixes some of the warnings on the code. This is not
> >        complete, and some of the casts are really important if you
> >        want xwisp2 to work under 64bit arches.
> 
> If not complete, what would be missing?

I fixed some of the warnings, but there are a lot of other warnings
in the code.

The main source of warnings are using a '%lu' modiffier in a printf
statement and passing and int argument. For exameple, in file xwisp2bus.c,
function WbusHello, you define:
  int    rc;
End then, you use:
  sprintf(szBuffer, "Communications error, rc %lu", rc);

In 64bit architectures (as AMD64), 'long' is 64bits and 'int' is 32bits,
so that won't work. To fix it, you can use '%d' in the printf (correct
for an 'int' argument), or cast the argument to 'unsigned long', or
cast the argument to 'unsigned' and use '%u' in the printf.

As I didn't know what is your prefered output format, I just added the
casts, but I think that the correct way is simply using '%d' in the
printf's.

> >+#if !defined(__LINUX__) && !defined(__linux)
> 
> May I suppose that '__linux' is defined by the Linux flavour of GCC?

Yes, it's defined by GCC. On Linux, you can type "cpp -dM < /dev/null"
to get a list of predefined symbols, the most usefull are:
  #define __linux 1
  #define __linux__ 1
  #define __unix 1
  #define __unix__ 1
  #define linux 1
  #define unix 1

The last two are *not* defined if you compile with strict-ansi
conformance (using the -ansi switch).

> >-#include <sys\types.h>
> >-#include <sys\stat.h>
> 
> The forward- in stead of back-slash works for the Open Watcom C compiler 
> and for the IBM Visual Age C compiler under OS/2 too! So I can safely 
> change that!

That's great!.

> >+  #define _searchenv(a,b,c) (*c=0)
> 
> Is there no _searchenv() functionality in Linux?

No, there is no standard function for "path searching". In Linux, you
normaly store config files in common locations, like
"/etc/myprog/myfile.cfg", so you don't search it in the path.

For an easy fix, I simply defined the searchenv function to return an
empty string, so the rest of your code works. A better solution would be
to write a searchenv function that search's in "/etc" and your home
directory for the files. I could do that if it's usefull.

> >-    sprintf(szBuffer, "WbusNext failed, rc %lu", rc);
> >+    sprintf(szBuffer, "WbusNext failed, rc %lu", (unsigned long)rc);
> 
> When 'rc' is declared as 'int', wouldn't is be better to use:
>   sprintf(szBuffer, "WbusNext failed, rc %d", rc);

Yes, but as that was not my code, I simply fixed it in the least
intrusive way (so the output remained the same) :-)

> >-extern int main(int argc, unsigned char **argv) {
> >+extern int main(int argc, char **argv) {
> 
> I never understood the difference between a pointer to a signed or 
> unsigned character, unless there are to me unknown architectures which 
> have a different length for these. Never mind.....

The importance is when you actually use the pointed value, for example:

  unsigned char *up = malloc(1);
  char *sp = malloc(1);
  int i;
  
  *up = 0xE0;
  *sp = 0xE0;

  i = *up; // Here, i is 224
  printf("i=%d\n",i);
  i = *sp; // Here, i is -32
  printf("i=%d\n",i);

Gcc uses signed chars as default, so null-terminated strings are signed
char pointers. The C standard don't specify any default, but I think
that signed chars are more common.

> 
> Regards, Rob.

Thanks for your work,

        Daniel