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

Add static size checks #6

Closed
DanRStevens opened this issue Mar 15, 2019 · 3 comments · Fixed by #30
Closed

Add static size checks #6

DanRStevens opened this issue Mar 15, 2019 · 3 comments · Fixed by #30

Comments

@DanRStevens
Copy link
Member

@DanRStevens DanRStevens commented Mar 15, 2019

Structs and classes exported by Outpost2.exe can have static size checks added to ensure link compatibility.

@DanRStevens

This comment has been minimized.

Copy link
Member Author

@DanRStevens DanRStevens commented Dec 8, 2019

Ok, I checked into this. The structs all have static size checks, so that part looks finished.

The classes should also have static size checks added. To the compiler, there is little distinction between class and struct, so what applies to one should probably be applied to the other. If class definitions result in the wrong binary size, that can also cause crashes and undefined behaviour.


I don't know that we need to add #pragma pack everywhere. The platform and bit size are all fixed for this project, so I wouldn't expect changes in alignment and padding, and kind of doubt a future version of the compiler will change that. I think we can do without the directive unless we run into compile errors. In particular, if there were changes to alignment and padding, the static_assert size checks would catch that, so we would know to add #pragma pack.

@Brett208

This comment has been minimized.

Copy link
Member

@Brett208 Brett208 commented Dec 9, 2019

A couple structs already had pragma pack already, so I just extended that. I don't mind removing them. Like you mentioned, it should complain if it is needed on a particular struct or class.

@DanRStevens

This comment has been minimized.

Copy link
Member Author

@DanRStevens DanRStevens commented Dec 9, 2019

Yeah, in the past, I may have used #pragma pack in a few places where it wasn't strictly needed. It's likely needed for the CommandPacket struct, as that has a number of non-naturally aligned fields. It's not likely going to change anything for structs where all fields are 32-bit. I probably used it more liberally than needed just to be consistent, or for vague unspecified reasons that I never thought clearly about.

I could go either way with continuing to use #pragma pack, though I think I'm leaning slightly towards only using it when it's absolutely required. Especially now that we have static_assert to warn us of problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.