Discarding typedef for enums?

[Anirban166]

If we are dealing with C++ and not C at any point (assuming, as the deployments have to do with instances of the components or C++ classes), can we discard the typedef part when declaring enums? (e.g.)

(Given that C++ can achieve the goal of discarding the prepended use of enum keyword without explicitly creating an alias)

[astroesteban]

I wonder how much effort it would be to change those to enum class types for added type safety 🤔

[LeStarch]

enum class is one of those C++11 features that can cause issues on incomplete and old compiler versions.

[Anirban166]

This reminds me of constexpr, which although some places could find use in fprime, might not be suitable given it's the one from C++11.

[Anirban166]

I wonder how much effort it would be to change those to enum class types for added type safety 🤔

That would be better yes, and if we are concerned both about type safety and it being strongly-typed as well, then typed enums would be more appropriate:

enum enumName: <dataType> {  memberA, memberB, ... };

In addition to the assignment of a specific type, this would also avoid the slight verboseness of scoping that comes when using members (enumName::memberB for instance) from an enum class.

[LeStarch]

We could replace them with enums of the form:

enum Something {
   val=1...
}

without major change to the code. These typedef enum {} name; constructs come from old C style programming that we are moving away from.

[Anirban166]

Would you like me to do the replacements?
The changes would be fairly straightforward I think, but I wonder if all of the members in the enumeration would need to get assigned a value, like: (considering the e.g.)

enum QueueStatus {
                  QUEUE_OK = 1, //!<  message sent/received okay
                  ...,
                  QUEUE_UNKNOWN_ERROR = 10 //!<  Unexpected error; can't match with returns
                 };

or if only some of them should have values?

[LeStarch]

The rule we use is:

1. All members are explicitly assigned a value --or--
2. All members are left to have the compiler assigned a value.

In this case, none of them have values, so we can just write:

enum QueueStatus {
            QUEUE_OK, //!<  message sent/received okay
            QUEUE_NO_MORE_MSGS, //!<  If non-blocking, all the messages have been drained.
            QUEUE_UNINITIALIZED, //!<  Queue wasn't initialized successfully
            QUEUE_SIZE_MISMATCH, //!<  attempted to send or receive with buffer too large, too small
            QUEUE_SEND_ERROR, //!<  message send error
            QUEUE_RECEIVE_ERROR, //!<  message receive error
            QUEUE_INVALID_PRIORITY, //!<  invalid priority requested
            QUEUE_EMPTY_BUFFER, //!<  supplied buffer is empty
            QUEUE_FULL, //!< queue was full when attempting to send a message
            QUEUE_UNKNOWN_ERROR //!<  Unexpected error; can't match with returns
};

[LeStarch]

I am happy to review changes like this, but please try to do one enum or one file per PR so that I can easily perform the review!

[Anirban166]

The rule we use is:

1. All members are explicitly assigned a value --or--
2. All members are left to have the compiler assigned a value.

In this case, none of them have values, so we can just write:

enum QueueStatus {
            QUEUE_OK, //!<  message sent/received okay
            QUEUE_NO_MORE_MSGS, //!<  If non-blocking, all the messages have been drained.
            QUEUE_UNINITIALIZED, //!<  Queue wasn't initialized successfully
            QUEUE_SIZE_MISMATCH, //!<  attempted to send or receive with buffer too large, too small
            QUEUE_SEND_ERROR, //!<  message send error
            QUEUE_RECEIVE_ERROR, //!<  message receive error
            QUEUE_INVALID_PRIORITY, //!<  invalid priority requested
            QUEUE_EMPTY_BUFFER, //!<  supplied buffer is empty
            QUEUE_FULL, //!< queue was full when attempting to send a message
            QUEUE_UNKNOWN_ERROR //!<  Unexpected error; can't match with returns
};

That's reasonable - all or nothing!
And gotcha, we are going with the default case here where the compiler assigns each member with an integer, starting from 0 and incrementing by one.

I am happy to review changes like this, but please try to do one enum or one file per PR so that I can easily perform the review!

I'll follow that from now on (keeping in mind for all future PRs), for sure!

[Anirban166]

@LeStarch Michael, would it be better to do files per fprime subdirectory (wrt the root) for these type of changes? (or just for this specific change?)

Casually thinking about it, but it seems like there would be too many PRs in a repetitive fashion (even for the changes itself) if I were to do one file per PR for this.
For instance, the remaining files under the OS directory which would be applicable for this change are WatchdogTimer.hpp, File.hpp, Directory.hpp, Task.hpp, FileSystem.hpp, ValidateFile.hpp and ValidateFileCommon.hpp - now that would be 7 more PRs to just have this minor change done for all files under OS/!

[Anirban166]

It might also be better to make that small change in the .tmpl files that account for the template structure or the code segment in the autocoder (assuming^* that to be the source that generates them, could be code in FPP too ig) which produces the 'typedef enum' constructs for the .cpp/.hpp files. (to keep things consistent for future autogenerated C++ code if this change is considered across all files)

The generation for the enum constructs (along with other model-based autocoder-emitted code) in the current version of fprime appears to be from FPP to XML (stored somewhere in the build cache), and then XML to C++, parsed and generated through the Python scripts or the autocoders (although it seems there have been efforts to remove the XML in between, and the FPP-based model itself would be used to directly generate the enums in the C++ files in the long run - like not integrated into the fprime build system yet, but not too far off maybe?)

While going through the Math Component tutorial, I noticed that the .fpp file specification included just an 'enum', but for what I saw, the automatically generated header file MathOpEnumAc.hpp (after building the model) had a 'typedef enum' for the same enum schema (and from the looks of it, it seems to be following the template (hpp) for it (and the other .tmpl files with 'typedef enum' constructs that I traced are cpp, enum_hpp, array_hpp, SerialH and namespacePortH - just for my reference). ^*I'm not sure what generates that part of the .cpp/.hpp code, but I'm guessing it's the autocoder since the fpp specs didn't have the keyword 'typedef'!

[LeStarch]

You are likely right. We could take a look at the old Python autocoder, but it is slowly being changed over to FPP anyway. So these constructs should disappear over time.