[MPlayer-dev-eng] [PATCH] Windows DLL support for OS X/Intel (cleaned version)

[Reimar Döffinger]

Hello,
On Sun, Nov 19, 2006 at 01:33:05PM -0500, Nicolas Plourde wrote:
>  echocheck "OpenGL"
>  #Note: this test is run even with --enable-gl since we autodetect $_ld_gl
> -if (test "$_x11" = yes || win32) && test "$_gl" != no ; then
> +if (test "$_x11" = yes || win32 && test "$_macosx" = no) && test "$_gl" != no ; then

Unrelated.

> +/*
> + * mmap_anon.c: provide a compatible anonymous space mapping function
> + */

These should all be doxygen-compatible.

> +#ifdef MAP_ANON
> +#ifndef MAP_ANONYMOUS
> +#define MAP_ANONYMOUS MAP_ANON
> +#endif
> +#endif

#if defined(MAP_ANON) && !defined(MAP_ANONYMOUS)
#define MAP_ANONYMOUS MAP_ANON
#endif

> +#ifdef __APPLE__

IMO #ifdef MAP_ANONYMOUS would be preferable, but others might disagree.

> @@ -28,6 +28,7 @@
>  #include <sys/types.h>
>  #include <stdio.h>
>  #include <unistd.h>
> +#include <osdep/mmap_anon.h>

Since it's not a system header this should be
#include "osdep/mmap_anon.h"
in all other places, too.

> @@ -104,6 +105,7 @@
>  /* i got this value from wine sources, it's the first free LDT entry */
>  #if defined(__FreeBSD__) && defined(LDT_AUTO_ALLOC)
>  #define       TEB_SEL_IDX     LDT_AUTO_ALLOC
> +#define       USE_LDT_AA

unrelated.

> -    ldt_fs->fs_seg = mmap(NULL, getpagesize(), PROT_READ | PROT_WRITE, MAP_PRIVATE,
> -			  ldt_fs->fd, 0);
> +	ldt_fs->fs_seg = mmap_anon(NULL, getpagesize(), PROT_READ | PROT_WRITE, MAP_PRIVATE, 0, 
> +	          &ldt_fs->fd);
> +

Those tabs are ugly and do not fit the rest of the code. And that empty
line should not be added, too.

> @@ -234,7 +237,7 @@
>          unsigned long d[2];
>  
>          LDT_EntryToBytes( d, &array );
> -#if defined(__FreeBSD__) && defined(LDT_AUTO_ALLOC)
> +#ifdef USE_LDT_AA

unrelated, too.

> -#ifdef MAP_PRIVATE
> -	flags |= MAP_PRIVATE;
> -#endif
> +		ret = mmap_anon( start, size_low, prot, flags, offset_low, &fd );

surrounding code uses spaces, not tabs.

> -    answer=mmap(NULL, len, mmap_access, MAP_PRIVATE, hFile, 0);
>      if(anon)
> +        answer=mmap_anon(NULL, len, mmap_access, MAP_PRIVATE, 0, &hFile);
> +    else
> +        answer=mmap(NULL, len, mmap_access, MAP_PRIVATE, hFile, 0);
> +
> +    if(anon && hFile != -1)
>          close(hFile);

Hmm. Shouldn't that be only
if(hFile != -1)
?

> -	if(anon)
> +	if(anon && hFile != -1)
>  	    close(hFile);

Same here. Since mmap_anon will open /dev/zero anew for each mmap, won't
it?
Should also extra-check that no new leaks are introduced.

Greetings,
Reimar Döffinger