OutpostUniverse / OP2Utility

C++ library for working with Outpost 2 related files and tasks.
MIT License
4 stars 0 forks source link

Add `static_assert` for `sizeof` structs used directly in serialization #191

Closed DanRStevens closed 6 years ago

DanRStevens commented 6 years ago

The size of a struct is not well specified by the C++ standard. It often depends on compiler and platform settings. In particular, alignment requirements of the platform may cause the compiler to insert padding bytes either before fields in a struct, or at the end up a struct, which may affect spacing in arrays of structs. This padding affects the output returned by the sizeof operator.

To guard against possible problems, we can add static_assert lines which verify an expected sizeof for the struct. Such checks can be placed directly after the struct definition so they can be kept in sync and verified easily.

The static_asserts should provide early warnings of problems on new platforms or compilers, and give people a chance to insert the necessary size or alignment directives (which are compiler specific).


Example:

struct SomeStruct {
  uint32_t a;
  uint32_t b;
  uint32_t c;
};
static_assert(12 == sizeof(SomeStruct), "Unexpected size of SomeStruct");

We might also consider using a(n evil) macro with the string pasting operator to report the name of the struct without duplicating it. Or we could omit the name and rely on the compiler reporting the line number to make it obvious. An example macro use might be:

VerifyStructSize(12, SomeStruct);
Brett208 commented 6 years ago

So adding #pragma pack(push, 1) / #pragma pack(pop) is a MSVC only solution? That stinks.

I would steer clear of the macro because we would have to add an include for some new header file to all the structure headers to define it. We don't have a global header like this yet, and I'd like to avoid making one if we don't need to.

This should be an easy project though. We can test the static_assert using the BmpHeader struct as it should fail if the preprocessor directive is removed on MSVC.

-Brett

DanRStevens commented 6 years ago

Hmm, it seems #pragma pack(push, 1) / #pragma pack(pop) started out as MSVC specific syntax, but were ported to GCC.

I would like to avoid macros simply because they are macros. Though to be fair, the structs used for serialization all tend to #include <cstdint> for the fixed sized integer types, so it wouldn't be much different in terms of includes.

Maybe we could do something similar with a template, though I doubt that would be able to automatically add the type name to an error string.

To keep things simple, maybe just a little bit of inline duplication is fine.