Skip to content

Conversation

@th-otto
Copy link
Contributor

@th-otto th-otto commented Mar 20, 2024

No description provided.

Copy link
Contributor

@OmniBlade OmniBlade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments apply to both games code, seems like a lot of changes to just silence a warning?

*/
#ifdef VIRGIN_CHEAT_KEYS
#define PARM_PLAYTEST 0xF7DDC227 // "PLAYTEST"
#define PARM_PLAYTEST 0xF7DDC227UL // "PLAYTEST"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint32_t is not unsigned long on all platforms.

{
Target.Sub.Exponent = RTTI_NONE;
Target.Sub.Mantissa = -1;
Target.Sub.Mantissa = (1UL << TARGET_MANTISSA) - 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is equivalent?

* 08/19/1995 JLB : Created. *
*=============================================================================================*/
long Obfuscate(char const* string)
uint32_t Obfuscate(char const* string)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The long type needs to go as its width varies between platforms but it should just change to int to match the ra code?

@th-otto
Copy link
Contributor Author

th-otto commented Apr 15, 2024

It's not only to get rid of the warning. Obfuscate() currently operates on an int, but in TD returns a long (unlike RA, which currently returns an int). Comparing that to a constant like 0xF7DDC227 may fail if the function actually returned 0xFFFFFFFFF7DDC227 because it was sign-extended to long. The only safe way is to declare them to return unsigned int (or uint32_t in this case), since this is what they actually calculate.

@OmniBlade
Copy link
Contributor

I'd be happy to see it changed to unsigned int in both cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants