[GRASS-dev] Re: [Qgis-developer] [patch] MSVC patch (less warnings and GRASS)

[Glynn Clements]

Jürgen E. Fischer wrote:

> > AFAICT, GRASS uses __STDC__ in a small number of headers to decide
> > whether to use ANSI-style prototypes. This is pointless; if you have a
> > compiler which doesn't understand them, you aren't going to be able to
> > compile GRASS.
>  
> > I'll fix both of these issues today.
> 
> Thanks.
> 
> Unfortunatley the MinGW DLLs didn't seem to work as well as I hoped.  I
> decided to build them with MSVC - with CMake out of the QGIS sources.
> To do that I had to modify the libs a bit.
> 
> The patch isn't probably of any use to GRASS itself, but AFAICS it
> doesn't do any harm either (not sure about the __BEGIN_DECLS/__END_DECLS
> stuff).
> 
> What do you think?

I've committed all the "harmless" changes, i.e. removing the
__{BEGIN,END}_DECLS and C99 variable declarations, and changing
strcasecmp() to G_strcasecmp().

I'm not keen on some of the other changes. I'd rather keep portability
issues in specific library functions than sprinkled around.

E.g. the S_ISDIR() stuff suggessts that we need a G_is_directory()
function. Similarly, a G_rmdir() function.

I'm not sure about the rewinddir() issue:

> Index: lib/db/dbmi_base/dirent.c
> Index: lib/gis/list.c

> +#ifndef _MSC_VER
>      rewinddir(dp);
> +#else
> +    closedir(dp);
> +    dp = opendir(dirname);
> +#endif

AFAICT, Windows itself doesn't have any of the dirent functions;
they're normally provided by some form of compatibility layer. In that
case, it would be preferable to use a library which supports
rewinddir().

Or failing that, just unconditionally re-open the directory rather
than only doing so on Windows; I doubt that efficiency is that great a
priority.

Both re-opening the directory and rewinddir() create a race condition,
i.e. you could get more entries on the second pass. G_list() will
typically segfault if this happens. It would be better to change the
code to dynamically re-allocate the array, eliminating the need to
re-scan the directory.

The chmod() call in lib/db/dbmi_base/login.c should probably just be
skipped on Windows. The purpose is to prevent other users from reading
the password stored in that file, so the Windows version is rather
pointless:

> +#ifdef _MSC_VER
> +    _chmod( file, S_IREAD | S_IWRITE );
> +#else
>      chmod ( file, S_IRUSR | S_IWUSR );
> +#endif

Creating a proper ACL for that file would be better, but I don't know
how to do that.

Regarding the DBMI RPC mechanism:

> Index: lib/db/dbmi_base/xdr.c

> -#ifdef __MINGW32__
> +#if defined(__MINGW32__) && !defined(_MSC_VER)
>  #define USE_STDIO 0
>  #define USE_READN 1
>  #else

This change was made because the MSVCRT fread() and fwrite() functions
didn't seem to work on pipes. We observed data corruption, which went
away when switching to read() and write(). I have no idea why this
would be any different when compiling with MSVC; have you checked that
using stdio actually works?

This:

> Index: lib/gis/G.h

> +#ifdef _MSC_VER
> +#include <basetsd.h>
> +typedef SSIZE_T ssize_t;
> +#endif //_MSC_VER
> +

doesn't belong in G.h, as that file doesn't reference ssize_t.

AFAICT, it's only used in get_row.c. The MSDN documentation says that
their read() returns "int", so we should probably use that:

	http://msdn2.microsoft.com/en-us/library/wyssk1bs(VS.80).aspx

> Index: lib/gis/error.c
> Index: lib/gis/tempfile.c

> +#ifdef _MSC_VER
> +#include <windows.h>
> +#define getpid() GetCurrentProcessId()
> +#endif

This suggests a need for a G_getpid() function.

> Index: lib/gis/parser.c

What about G_parser() is causing problems with MSVC? Obviously, we
can't disable that function in GRASS itself.

> Index: lib/gis/spawn.c
> Index: lib/gis/system.c

>  #ifndef __MINGW32__
>  #include <sys/wait.h>
> +#else
> +#include <process.h>
>  #endif

For legibility, the sense should probably be switched, i.e.:

	#ifdef __MINGW32__
	#include <process.h>
	#else
	#include <sys/wait.h>
	#endif

Or the tests should be separated, i.e.:

	#ifndef __MINGW32__
	#include <sys/wait.h>
	#endif
	#ifdef __MINGW32__
	#include <process.h>
	#endif

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