x86_64 patch

[DevUrandom]

Aha... Didn't know that, yet...

[pseudonym404]

Just updated to current svn, no problems with using char* instead of void* (wasn't aware msvc couldn't do pointer maths with void*)

I was thinking the next thing would be to clean up all of the remaining pointer/integer cast warnings, starting with the use of the WIDGET_BASE member pUserData for storing integers (i don't think this is actually causing issues on amd64, but does seem to be responsible for the majority of warnings while compiling NDEBUG).

I'm undecided whether to simply change the casts to eliminate the warnings, or add an integer type (or find out if it's ok to use UserData) for integer widget user data, any thoughts?

[DevUrandom]

Limit casts to a minimum. I really don't like the idea of silencing the compiler by adding some more casts... That only hides potential bugs. Always try to fix the variable instead. (And just do the casts if it is intended to work that way.)

[pseudonym404]

Couple more patches.

warzone-imdload.patch is just a warning cleanup, assign pointers to NodeList elements in the first loop, instead of assigning the id and looping through again.

warzone-hashtable.patch changes the first hashtable key to long (pointers being used as unique ids), and removes unneeded functions. I can do this differently if there's some reason I'm not aware of for keeping those functions around

[DevUrandom]

imdload:
- Pointers are NULL (at least GCC does additional checks for NULL, but not for 0)
- Comments should be kept
(I'll fix that when commiting.)

hashtable:
- Don't use long, but UInt64 or similar instead, please. This way we can easily change the types afterwards, so they are all the same in all files.
- You said that those are pointers being used as IDs. Why can't we give them the type of a pointer, but need to use long/UInt64 instead?

[pseudonym404]

- Pointers are NULL (at least GCC does additional checks for NULL, but not for 0)
- Comments should be kept

Yep, I should have changed the 0 to NULL, missed that one. The comment I removed didn't seem relevant with the two loops merged though..

- Don't use long, but UInt64 or similar instead, please. This way we can easily change the types afterwards, so they are all the same in all files.

OK

- You said that those are pointers being used as IDs. Why can't we give them the type of a pointer, but need to use long/UInt64 instead?

We could, and cast to uint64 when needed, but as it's never cast back to a pointer once it's passed to a hashtable function, I though an integer type made more sense.

The one point where the value is actually used is in hashTable_GetHashKey, where the modulous of the two keys with the table size (actually the nearest prime to the table size) is returned. The reason for changing this bit of code is to avoid the possibility of returning the wrong hashtable entry if the least significant 32 bits of two or more 64 bit pointers happen to match.

[DevUrandom]

Yep, I should have changed the 0 to NULL, missed that one. The comment I removed didn't seem relevant with the two loops merged though..

It seemed to me that the comments explain a bit of the tree, so it might be usufull if someone wants to find out how it works.

We could, and cast to uint64 when needed, but as it's never cast back to a pointer once it's passed to a hashtable function, I though an integer type made more sense.

The one point where the value is actually used is in hashTable_GetHashKey, where the modulous of the two keys with the table size (actually the nearest prime to the table size) is returned. The reason for changing this bit of code is to avoid the possibility of returning the wrong hashtable entry if the least significant 32 bits of two or more 64 bit pointers happen to match.

So this key1 is only used in one function? But it gets returned to a function who handles it as an int and not as a pointer?
So that function needs to be also changed? Or did I get this wrong and the key is only used internally by the hashtable?

[pseudonym404]

The key is only used internally by the hashtable, and as an integer, not a pointer

edit: basically, the hashtable wants a unique integer, for which the value of a pointer is being used, so the integer type needs to be large enough to contain that value.

[DevUrandom]

I think from earlier discussions with Per I remember that from the hastable only the first key is really ever used. Could you check that?
And where is key1 used as an int? Maybe we can change the whole hashtable to operate on 1 pointer instead of 2 int?

Actually, what is this one?

Code: Select all

iHashValue = ( iHashValue << ONE_EIGHTH ) + *c;

Dereferencing the pointer which was proviously stored in an int? What does it point to?

[pseudonym404]

I think from earlier discussions with Per I remember that from the hastable only the first key is really ever used. Could you check that?
And where is key1 used as an int? Maybe we can change the whole hashtable to operate on 1 pointer instead of 2 int?

projectile.c only uses the first key, animobj.c passes a second key, but doesn't really need to.
key1 is used in hashtable.c hashTable_GetHashKey like:

Code: Select all

return (iKey1 + iKey2) % psTable->udwTableSize;

edit: without the changes in warzone-hastable.patch, key1 and key2 are added in a callback function called from hashTable_GetHashKey, but as these callbacks served the same purpose, I removed those too.

Actually, what is this one?

Code: Select all

iHashValue = ( iHashValue << ONE_EIGHTH ) + *c;

Dereferencing the pointer which was proviously stored in an int? What does it point to?

That line was part of the HashPJW function I removed, as it wasn't being used, as HASHTEST was defined, and both animobj.c and projectile.c, the only things using the hashtable, have comments that show they were intended to use the ((key1 + key2) % nearest prime to table size) hashing method.

*c would point to the passed object if that method were to be used, resulting in a hash of the object itself rather than it's memory address.

[DevUrandom]

So in the end the only place where the key is used as an int is in the following?

Code: Select all

return (iKey1 + iKey2) % psTable->udwTableSize;

Can't we do some math with the pointer there? (If compilers spit out warnings we could still cast.)
Can't we strip the 2nd key and in places where it is needed simply add (key1+key2) it directly when passing it over to the hashtable functions?

Actually I still didn't have a look on how this hashtable stuff really works, where that GetHashKey is used and what happens with the keys internally, so you might want to tell me that all I've been talking is impossible.

[pseudonym404]

So in the end the only place where the key is used as an int is in the following?

Code: Select all

return (iKey1 + iKey2) % psTable->udwTableSize;

Yep

Can't we do some math with the pointer there? (If compilers spit out warnings we could still cast.)

% isn't a valid operator for pointers, so we'd wind up with:

Code: Select all

return (uintptr_t)key % psTable->udwTableSize;

(or whatever type we've defined to use for casting pointers to integers)

Can't we strip the 2nd key and in places where it is needed simply add (key1+key2) it directly when passing it over to the hashtable functions?

I think so, yes.

If someone could playtest on x86 when this bit's done? Just to be completely sure this isn't breaking anything before continuing, as I'm still seeing glitches I can't attribute to this on amd64 (changing this bit appears to have solved a random fireworks display and spinning camera glitch I was seeing when attacking things previously though). If it's going to cause problems, it's going to be strange behaviour when firing at stuff..

[DevUrandom]

Which bit?

[pseudonym404]

Hashtable changes

Is the attached patch what you mean?

[DevUrandom]

Looks ok so far... Maybe we should get some comments in, telling that the key is some type of pointer which may be modified by a modifier to get a hashkey.

Apart from that I only found 2 places where I still have questions:
1.

Code: Select all

#define	UNUSED_KEY	-747

Would it matter to "set" this to 0 by simply not adding UNUSED_KEY? Or is this added to prevent some clash with animobj? (Is it possible anyway to prevent such clash when adding varying numbers to pointers???)
2. Where does that __WORDSIZE come from? Is that set on all compilers / systems? And shouldn't this be about pointersize? (I interprete WORD to be some int type, am I wrong?)