Use of enum variable for bit flags prevents compilation as C++ source

[smuehlst]

I'm compiling OpenJPEG with the .NET compiler in C++ mode. There are two problems that prevent successful compilation out of the box. One is missing casts for return values of opj_malloc()/opj_calloc(). For that I already created pull request #618.

The other problem is the use of an enum typedef for a variable that is intended for holding bit flags. From src/lib/openjp2/cio.h:

typedef enum
{
        opj_signed_sentinel     = -1, /* do not use in code */
        opj_stream_e_output     = 0x1,
        opj_stream_e_input      = 0x2,
        opj_stream_e_end        = 0x4,
        opj_stream_e_error      = 0x8
}
opj_stream_flag ;

typedef struct opj_stream_private
{
    ...
    /**
     * Flags to tell the status of the stream.
     */
    opj_stream_flag m_status;
}
opj_stream_private_t;

And in src/lib/openjp2/cio.c for example:

l_stream->m_status |= opj_stream_e_input;

This fails to compile in C++ mode. The declaration of the m_status member as opj_stream_flag in my view does not add value here, because as the variable can contain a combination of the opj_stream_* flags it is not restricted anyway to the actual members of theopj_stream_flag enumeration.

To be able to compile in C++ mode I changed the type of the m_status variable from opj_stream_flag to OPJ_INT32 in my local repository. Would you consider to apply this change? If so, I would create a corresponding pull request.

[mayeut]

@smuehlst,

I guess the flags should all be declared as preprocessor defines:

#define OPJ_STREAM_STATUS_OUTPUT  0x1U
#define OPJ_STREAM_STATUS_INPUT   0x2U
#define OPJ_STREAM_STATUS_END     0x4U
#define OPJ_STREAM_STATUS_ERROR   0x8U

The structure shall be defined as:

typedef struct opj_stream_private
{
    ...
    /**
     * Flags to tell the status of the stream.
     */
    OPJ_UINT32 m_status;
}
opj_stream_private_t;

I think it's best to use unsigned integers for bitfields (some static analyzers will report a warning for bitwise operations on signed integers).

[smuehlst]

@mayeut: I see, you are right that an unsigned integer makes more sense. I did not pay attention that the enum member opj_signed_sentinel Isn't actually used anywhere.

Can I change the sources according to your above proposal and send a pull request?

[stweil]

@smuehlst : Why not add a | operator for the enum in question? Here is an example:
http://stackoverflow.com/questions/19337064/enum-invalid-conversion-from-int-in-class

Of course the operator would need conditional compilation (only for C++).

[smuehlst]

@stweil: I wouldn't want to spoil the code with any C++ constructs for this purpose, and you would have to ask the OpenJPEG developers whether they want this.

Do you see any value in the declaration of m_status as an enum typedef? Or do you see any downside in declaring m_status as an unsigned int?

[mayeut]

@smuehlst,
I think you can go ahead with OPJ_UINT32 or unsigned int and create pull request.
This is only in private headers & won't change API/ABI. If the PR passes travis-ci tests, I do not see a reason not to merge it.

As you said, do not use C++ constructs.

[mayeut]

@malaterre,
From my understanding, bitwise operations in C behave the same with signed or unsigned integers as they operate directly on the bit representation.
The only exception is for << and >> shift operators for which behaviour is implementation defined or undefined for signed integers (maybe, depending on the sign...). However on most common platforms / compilers, the behaviour is well defined (& often optimizations rely on 2's complement).
I don't see why we'd get any warnings using unsigned int. However, I know of static analyzers that will generate warnings on all bitwise operations with an operand of signed int type.

[malaterre]

@mayeut sorry for the noise, using unsigned int is the right thing. Mixing int and unsigned int will generated warning. But we just need to make sure enums are set to unsigned int and be done.

Eg:

/* gcc -Wconversion t.c */
int main()
{
  unsigned int i = 0;
  int v = 3;
  i |= v;
  return 0;
}

[stweil]

@mayeut, I don't think that C++ code in header files is a problem, as long as it is encapsulated with #ifdef __cplusplus ... #endif. This is quite common for system header files.

I'd keep the enum type because it is more safe and accept a few lines of C++ (as in my pull request) instead of using other integer types.

@smuehlst: My pull request #621 also fixes some more issues for C++ and passes compilation with g++. Perhaps you can try it with your compiler, too.

[smuehlst]

@mayeut: I created pull request #622 with the changes that you proposed.

@stweil: Thanks for the patch, but I personally consider the introduction of C++ operators overkill. The use of an enum type is actually misleading in my view, because the variable can contain a combination of the bit flags, so the actual value of the m_status variable can be different from any of the four defined enum members.