x86_64 patch

[pseudonym404]

1.
I've only left UNUSED_KEY in so that the hashtable should function the same after as before the changes, I'm not entirely sure why it's -747... we could experiment with dropping the second key entirely in both cases..

2.
bits/wordsize.h, which is included by stdint.h, which is why there's a default to 32 if it's undefined
I used __WORDSIZE as that's what stdint.h uses for it's uintptr_t typedef and as it's type size specific rather than x86_64 specific
Could alternatively do it like this:

Code: Select all

#ifdef __intptr_t_defined
typedef uintptr_t UPWORD
#else
typedef Uint32 UPWORD
#endif

Or stick with the #ifdef __x86_64__ I originally used (which is what bits/wordsize.h uses to define __WORDSIZE (edit: on an x86_64 system of course))

[DevUrandom]

Doesn't MSVC miss stdint.h? So we'd need some workaround for it...
And: I urge you to move that stuff over to platform.h, as that's the place I created for all those OS and platform specific stuff. You may want to have a look at wzglobal.h to see if you can use some of those defines for your task.

[pseudonym404]

Which is why there's a:

Code: Select all

#ifndef __WORDSIZE
#define __WORDSIZE 32
#endif

to define __WORDSIZE if it's not already defined.

It feels like we're going round in circles on this one.. I have, at no point, with any of these methods, depended on stdint.h, there's always been a fallback to using a 32 bit type. It really doesn't matter which method gets used, and by all means, move it to platform.h

edit: is the attached patch better?

[DevUrandom]

I don't want to pick on your patch or anything, but if __WORDSIZE is not defined (may be the case on MSVC, I really don't know) it is defined to 32. But what if MSVC compiles for a 64bit system? Can perhaps someone with experience in MSVC give a hint on how to find out whether we compile for 64bit?

Qt uses

Code: Select all

#if defined(Q_OS_WIN64)
# define QT_POINTER_SIZE 8
#elif defined(Q_OS_WIN32)
# define QT_POINTER_SIZE 4
#endif

and

Code: Select all

#if defined(Q_OS_WIN) && !defined(Q_CC_GNU)
typedef __int64 qint64;            /* 64 bit signed */
typedef unsigned __int64 quint64;  /* 64 bit unsigned */
#else
typedef long long qint64;           /* 64 bit signed */
typedef unsigned long long quint64; /* 64 bit unsigned */
#endif

and for QT3_SUPPORT:

Code: Select all

#if defined(Q_OS_WIN64)
typedef __int64 Q_LONG;             /* word up to 64 bit signed */
typedef unsigned __int64 Q_ULONG;   /* word up to 64 bit unsigned */
#else
typedef long Q_LONG;                /* word up to 64 bit signed */
typedef unsigned long Q_ULONG;      /* word up to 64 bit unsigned */
#endif

but I wonder what this really means. (Esp. if the OS is not WIN or the CC is GNU (GCC))
Is long long (and long) on other systems of the same size as a pointer? Probably not, I guess...

I am sorry if this looks like I want to hold your patch back or anything. Really: I'd love to include it! I just need to make sure that this works as intended on all supported systems. And in this case I am not...

EDIT:
I just found /usr/include/SDL/SDL_config_win32.h (which should be simply named SDL_config.h on WIN32/MSVC systems), which seems to tell that uintptr_t and all the other stdint.h types are defined by SDL on Windows systems and http://www.libsdl.org/release/SDL-devel-1.2.11-VC6.zip seems to prove it. Can someone confirm that this works?

[Troman]

Can perhaps someone with experience in MSVC give a hint on how to find out whether we compile for 64bit?

We can use _WIN64 and _M_X64, looks like _M_X64 is only defined for AMD64.

[pseudonym404]

Ahh, it all makes sense now. I simply hadn't even considered 64 bit windows.

edit: *fixed mental image of 'win32'*

[krushia]

What is the status of the x86_64 fixes?  I'm trying to build and I get lots of integer warnings, and it dies when it tries to bind SDL_net and libphysfs.

[pseudonym404]

Sorry, I got sidetracked by other things (including moving house).

Current SVN is more or less playable, a few glitches still.

Attached patch fixes loading/saving fx data on x86_64 (was triggering an assert when loading some save games)

I'm going to have a look at the remaining pointer/integer cast warnings and see what I can do (there's got to be a neater way of tidying up the hashtabl/animobj code than the previous patch I posted).

Looking at the menu code, I should be able to clean up the large majority of the remaining cast warnings simply by using the existant (in the WIDGET_BASE macro in lib/widget/widgbase.h) UDWORD UserData for integer data instead of the current situation of using void *pUserData for pointers and integers.

[pseudonym404]

Oh, and a couple of trivial warning cleanups

[Giel]

Oh, and a couple of trivial warning cleanups

Would you mind quoting the exact warnings they fix as well ?

Since I can't see how changing "unsigned int" -> "long" would fix/improve anything (yes it's larger but that shouldn't matter since we never need a larger range than that of an int). And (just nitpicking here), "unsigned long" would be the semantically correct type there .

As for the script parser. Aside from writing a "specialized" macro just to circumvent a warning. Wouldn't turning it in an inline function have the same effect (with less code as a result)?

Your scriptfuncs patch is perfectly fine though. Applied it in r2376.

[pseudonym404]

I really shouldn't post patches last thing at night

I'd configured the source with --enable-debug-relaxed (as --enable-debug hides pointer cast warnings, which are what I'm looking for), so -Wextra was enabled, so I was getting lots of "warning: comparison between signed and unsigned", 278 instances just from pievector.h

the change to long was entirely in the interests of improving the signal to noise ratio (although poorly thought out), I'd originally intended to ignore the majority of signed/unsigned warnings

The script parser, inline function is probably better, patch attached (warning circumvention primarily in the interests of not needing to use -Wno-non-null)

The loadfx/savefx patch in the previous post is the one that actually solves an x86_64 specific problem (stops loading save games triggering an assert in lib/framework/frameresource.c:resGetDataFromHash due to a mangling a hash value), the rest weren't really important

[Giel]

I really shouldn't post patches last thing at night

Neither should I read&apply them so late

Anyway your non-null fix patch is applied in r2377 with some changes by myself.

E.g. you used:

Code: Select all

#define ALLOC_VARIDENTDECL(psDcl, ident, dim) \
	CODE_ERROR err = do_ALLOC_VARIDENTDECL(&psDcl, ident, dim); \
	if (err != CE_OK) return err

While I changed it to:

Code: Select all

#define ALLOC_VARIDENTDECL(psDcl, ident, dim) \
{ \
	CODE_ERROR err = do_ALLOC_VARIDENTDECL(&psDcl, ident, dim); \
	if (err != CE_OK) \
		return err; \
}

Do you see the difference with those scoping symbols ? That's intentional to prevent poluting the scope in which that macro is used with an additional variable that's only used by the macro. Plus MSVC (that god damned *hell* compiler, which we support) only accept variable declarations at the start of a scope.

Further I just added some comments and did some rather minor changes.

PS Can I consider your pievector patch dropped (i.e. trashed) ?

PS2 Would you please use "svn diff" rather than just plain "diff" for generating your patches next times. That'll make it easier for me to automatically apply your patches to my working copy.

[pseudonym404]

Do you see the difference with those scoping symbols ? That's intentional to prevent poluting the scope in which that macro is used with an additional variable that's only used by the macro.

Makes sense (especially with such a common variable name as 'err')

Plus MSVC (that god damned *hell* compiler, which we support) only accept variable declarations at the start of a scope.

Ouch.. makes me glad I have no need to support MSVC in my own projects..

PS Can I consider your pievector patch dropped (i.e. trashed) ?

Yep

PS2 Would you please use "svn diff" rather than just plain "diff" for generating your patches next times. That'll make it easier for me to automatically apply your patches to my working copy.

OK.

I'm currently looking at cleaning up the large number of pointer/integer cast warnings in the menu code (should make it easier to see any remaining real problems afterwards), the vast majority of which are simply due to storing integer values in a void pointer (pUserData).. The simplest method (but ugly) would be to add a whole heap of explicit casts to an integer the same size as a void pointer before the cast to void*. Alternatively I could do something along the lines of adding a UDWORD iUserData for integer values (it seems buttons at least currently use both void *pUserData and the existant UDWORD UserData). Any better ideas?

[Per]

Plus MSVC (that god damned *hell* compiler, which we support) only accept variable declarations at the start of a scope.

That is a good idea in any case. It makes code much more readable. And it is the coding style followed in the rest of the codebase.

[Giel]

Plus MSVC (that god damned *hell* compiler, which we support) only accepts variable declarations at the start of a scope.

That is a good idea in any case. It makes code much more readable. And it is the coding style followed in the rest of the codebase.

I disagree. Not having to browse to the top of the function to see what type a variable is when you see it first used is a luxury IMO. Also the knowledge that the variable didn't exist before a certain line is nice to know, because that tells you it can't have undergone changes before that line/statement.

As for the coding style thing, I do agree that consistency increases readability.