OutpostUniverse / OP2Utility

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

Remove g++ warnings with workaround #176

Closed DanRStevens closed 3 years ago

DanRStevens commented 6 years ago

Relating to Issue #175, I did a bit of investigation into the current g++ warnings. An example warning is:

src/Maps/TileData.h:11:22: warning: ‘TileData::cellType’ is too small to hold all values of ‘enum class CellType’
  CellType cellType : 5;
                      ^

The compiler complains when enum class variables (aka "scoped enums") are packed into bit fields. Turns out, the warning stems from the base type of the enum, rather than enum class. It just so happens that if an enum class has no explicit base type specified, then it will have an implicit base type of int. Regular enum types may also have a base type specified, but do not have an implicit base type when unspecified.

The following would need both the class and the base type uint32_t removed to avoid the warning:

enum class VolPadding : uint32_t

I did some quick edits, and found simply removing the class and base types allowed code to compile without warnings or errors. We'd lose the scoped nature of the enums, though we eliminate compiler warnings.


Looking at this, I get the sense my explicit use of a base type was probably a bad idea. That's probably more useful when you're using an opaque enum.

An opaque enum is an enum declaration, without a corresponding definition of enum values. This is sometimes used in the public portion of an API declaration, while the actual enum values are kept private. Such a declaration allows you to pass enum values around as parameters, and declare variables to hold that type. Since the size is known, it's no problem for the compiler to reserve space for these fields, or generate code to move the values around. Actual use of the specific enum values would be reserved for internal use by the library. The client doesn't need to know what those values are, only that is can pass appropriate values along that it's been given.

We don't need opaque enums, nor are we using enums as opaque enums. It should be reasonable for use to remove explicit base types.


The more controversial part of removing the warnings, is to remove the class part, and turn the scoped enums into regular enums. This of course affects namespacing, as all enum value names now become part of the parent namespace.

We could possibly wrap enums in a namespace, or perhaps rely on a library namespace which is known to not have collisions.

DanRStevens commented 6 years ago

As it relates, here is a StackOverflow question about whether or not using the scope resolution operator (::) is standards compliant on regular enum types: Scope resolution operator on enums a compiler specific extension?

It seems it is/was non-standard, though common before C++11. I saw a comment in there that suggested C++11 added the ability to use scope resolution even on regular enums, though that comment didn't give a reference to anywhere.

If it is part of C++11, which we are explicitly using (or rather C++14), then any explicit use of scope resolution with enum classes wouldn't be impacted if the class part was removed. If it's a non-standard extension, then my little test that resulted in no warnings or errors might be less portable than I'd hoped.

Brett208 commented 6 years ago

I was thinking we should take care of this warning as well the other day. Since we have a static check to ensure the enum and bitfield will work together, wouldn't it be appropriate to just silence the warning with a preprocessor directive like we have done with an occasional MSVC warning? If they become incompatible in the future or with a different compiler, an error would be presented anyways.

Thanks, Brett

DanRStevens commented 6 years ago

From the thread concerning the compiler bug, there is no option to silence that warning. Though you're right, it would make sense to just silence the warning in such a case.

I was also noticing if we change the base type of the enum, it might change the struct sizes that use them, and that affects serialization. I'm not sure we have sufficient tests in place to catch such a change.

Perhaps we could add some static_asserts after the struct definitions to verify the sizeof matches the expected value. I was playing around with such checks for NetFix, where struct size is also very important to maintain compatibility with existing network packets.

DanRStevens commented 6 years ago

Just wanted to leave a link to the relevant GCC bug thread: Bug 51242 - Unable to use strongly typed enums as bit fields

And also a related StackOverflow question: gcc supress warning “too small to hold all values of”

DanRStevens commented 3 years ago

Good news, the GCC compiler warning was fixed in GCC 9.3: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51242#c31

Since upgrading the Docker image for CircleCI to be based on Ubuntu 20.04 (#339), it seems the warning has gone away.

I think that's sufficient to close this now.