Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix more sdktools crash on 64 bits #2152

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bottiger1
Copy link
Contributor

Found some more pointer arithmetic in sdktools assuming 32bits.

@KyleSanderson
Copy link
Member

is there still - 4 to get IClient?

@bottiger1
Copy link
Contributor Author

is there still - 4 to get IClient?

I did grep "+ 4" and "- 4" in the sdktools directory. I did not check any other directory.

@asherkin
Copy link
Member

asherkin commented May 9, 2024

While I understand it seems to be the same in practice, sizeof(intptr_t) feels wrong here - I would expect sizeof(void *) for this usage.

@assyrianic
Copy link
Contributor

While I understand it seems to be the same in practice, sizeof(intptr_t) feels wrong here - I would expect sizeof(void *) for this usage.

Any particular reason why? Most of the code itself is already using [u]intptr_t anyway.

@asherkin
Copy link
Member

Any particular reason why? Most of the code itself is already using [u]intptr_t anyway.

Which is obviously correct for the math, but the spec only appears to guarantee sizeof(intptr_t) >= sizeof(void *) - and what we want here is the size of a (specific) pointer, not the size of the integer type to represent any arbitrary pointer.

@bottiger1
Copy link
Contributor Author

We don't need to have a long discussion about this. If you want to change it to void* for a hypothetical architecture where the fastest integer type is somehow larger than the pointer type, it doesn't matter to me. I just want these crashes gone.

@dvander
Copy link
Member

dvander commented May 10, 2024

static_assert() that your intention is true and move on, I say.

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.

None yet

5 participants