billvaglienti / ProtoGen

Communications protocol generation software
MIT License
30 stars 16 forks source link

New 'flag' type #56

Closed jefffisher closed 6 years ago

jefffisher commented 7 years ago

C++ does not allow a logical OR on an enumerated type, resulting in an error like the following if you were to try and OR two values:

error: invalid conversion from 'int' to 'SomeEnum_t'

One option is to define inline functions to perform these functions. As an example, I hand-modified my generated SomeProtocol.h file to add the following block:

#ifdef __cplusplus
#define MAKE_FLAGS(Type) \
    inline Type  operator|  (Type  a, Type b) { return static_cast<Type>(a | b); } \
    inline Type &operator|= (Type &a, Type b) { return a = a | b; }
extern "C" {
#else
#define MAKE_FLAGS(Type)    // Don't change the behavior in C code
#endif

Then, for any enumeration I want to make a flag, all I have to do is add a MAKE_FLAGS expansion after the enumeration, like so:

typedef enum
{
    ENUM_VALUE_0 = 1,
    ENUM_VALUE_1 = 2,
    ENUM_VALUE_2 = 4
} SomeEnum_t;

MAKE_FLAGS(SomeEnum_t)

It doesn't always make sense to define an OR on an enumerated type, though, so I would propose adding a new XML attribute to the Data tag, like flags="SomeEnum_t", which would behave exactly like the enum attribute, but add the logical OR functions.

billvaglienti commented 7 years ago

I found an annoying wrinkle.

Ideally I would like to add the operator functions immediately after the definition of the enumeration. However if you define the operator in a header file, you can only have one. If you define another operator in a header file (even for a different enum type) the compiler complains about two functions of the same name. Interestingly this happens even if you move the operator definition outside the extern "C" block. (I figured the problem was lack of name mangling, but apparently not).

So it appears you have to declare the operator in a .cpp file. Perhaps you already knew this, and that is why you suggested a macro to define the operator. In any case if you have to declare the operator manually anyway, there seems little point in having Protogen output it.

FYI the operator definition needs to static cast the enumerations to , otherwise the code will segfault when it runs.

jefffisher commented 7 years ago

Looks like if you move all the macros to the bottom of the packet definition header file it works:

// SomePacket.h was generated by ProtoGen version 1.9.5.c

#ifndef _SOMEPACKET_H
#define _SOMEPACKET_H

// C++ compilers: don't mangle us
#ifdef __cplusplus
extern "C" {
#endif

/*!
 * \file
 * \brief SomePacket.h defines the interface for the SomePacket packet of the Some protocol stack
 *
 */

#include "SomeProtocol.h"

// [ ... ]

typedef enum
{
    ENUM_VALUE_0 = 1,
    ENUM_VALUE_1 = 2,
    ENUM_VALUE_2 = 4
} SomeEnum0_t;

// [ ... ]

#ifdef __cplusplus
}
#endif

MAKE_FLAGS(SomeEnum0_t)
MAKE_FLAGS(SomeEnum1_t)
MAKE_FLAGS(SomeEnum2_t)

#endif
billvaglienti commented 7 years ago

Yeah I already tried it, but it won't compile: the specific error is:

error: conflicting declaration of C function 'packetIds operator|(packetIds, packetIds)' inline Type operator| (Type a, Type b) { return static_cast(a | b); } \ ^ This is on mingw on windows.

jefffisher commented 7 years ago

Strange... I was testing in linux on gcc 4.3.3 and 4.6.3 and it worked fine. So I just tested again, this time in Windows with MinGW gcc v4.9.11 and it also seems to build without errors. Just to compare results with you, here are my new first lines in OrionPublicProtocol.h:

// OrionPublicProtocol.h was generated by ProtoGen version 1.8.2.b

#ifndef _ORIONPUBLICPROTOCOL_H
#define _ORIONPUBLICPROTOCOL_H

// C++ compilers: don't mangle us
#ifdef __cplusplus
#define MAKE_FLAGS(Type) \
    inline Type  operator| (Type  a, Type b) { return static_cast<Type>(a | b); } \
    inline Type &operator|=(Type &a, Type b) { return a = a | b; }
extern "C" {
#else
#define MAKE_FLAGS(Type)
#endif

And here are the final lines in my modified OrionPublicPacket.h:

#ifdef __cplusplus
}
#endif

MAKE_FLAGS(OrionCameraProtocol_t)

#endif

If you make those exact changes, do you still get compiler errors?

billvaglienti commented 7 years ago

No, but it will if I include a second MAKE_FLAGS for a different enumeration.

jefffisher commented 7 years ago

I changed OrionPublicPacket.h to include this:

#ifdef __cplusplus
}
#endif

MAKE_FLAGS(OrionCameraType_t)
MAKE_FLAGS(OrionCameraProtocol_t)

#endif

And I still don't get any compiler errors

billvaglienti commented 7 years ago

Are you compiling a .cpp file?

jefffisher commented 7 years ago

I'm just building the whole TrilliumApps project, which is a combo of C and C++ files. Those files are included, directly and indirectly in probably half of the source files in the entire project.

jefffisher commented 7 years ago

Okay, I finally got back around to this. I ran into way too many build errors in TrilliumApps to get it building in Qt 5.7, but I did manage to successfully build and run the MissionComputer/PathMapper project with my changes from above.