Bit fields type

[smuehlst]

The definition of bit-fields with type OPJ_UINT32 caused compilation errors
on IBM iSeries, because OPJ_UINT32 is defined as uint32_t, and
uint32_t is defined as unsigned long in <stdint.h> on IBM iSeries:

228       |typedef struct opj_simple_mcc_decorrelation_data                                                  
229       |{                                                                                                
230       | OPJ_UINT32    m_index;                                                                          
231       | OPJ_UINT32    m_nb_comps;                                                                       
232       | opj_mct_data_t *  m_decorrelation_array;                                                        
233       | opj_mct_data_t *  m_offset_array;                                                               
234       | OPJ_UINT32    m_is_irreversible : 1;                                                            
==========> .........................................a........................................................
=SEVERE==========> a - CZM0009  Bit field m_is_irreversible must be of type signed int, unsigned int or int. 
235       |}  

The definition of bit-fields with an integer type of a specific size doesn't make sense anyway. Therefore I introduced a new typedef OPJ_BITFIELD defined as unsigned int, and changed all bit-fields in structures to use this typedef.

[mayeut]

This type is only used by internal headers. Please do not define it in openjpeg public header.

[smuehlst]

@mayeut: What would be the right place then? Would it be in opj_includes.h?

[mayeut]

@smuehlst , yes opj_includes.h seems to be the right place for this.

[stweil]

Why is a new type needed? I'd simply use int, unsigned or an enum type (choice depending on the allowed values of the bit field).

[stweil]

I forgot bool which is also a reasonable type for a lot of bit fields.

[stweil]

I also forgot to mention that int, unsigned, bool and all enum types work for bit fields with standard C / C++ compilers. The QEMU project writes bit fields like this, and it also supports IBM iSeries, so I assume it would work for OpenJPEG, too.

[smuehlst]

@stweil: I just tried to follow the convention that OpenJPEG uses OPJ_ typedefs for all the basic types. Therefore it seemed to make sense to me to introduce a new type OPJ_BITFIELD. The additional benefit is that if there ever is an exotic compiler that has problems with the given definition of OPJ_BITFIELD, the type to use for bit-fields can be changed in a central place.

[stweil]

OpenJPEG also uses lots of int and unsigned type qualifiers. You won't find a compiler which is useful for OpenJPEG and which does not support the basic C / C++ standard for bit fields, so it is not necessary to introduce a new typedef. In addition you lose flexibility if you restrict bit fields to OPJ_BITFIELD: no signed values, no boolean values, no enum values which are all useful because they allow more compiler checks and optimizations.
Your reason to fix a compilation problem and replace OPJ_UINT32 (which is indeed wrong here) for bit fields is good, but IMHO we should not use the replacement which you suggested.

[stweil]

I must correct myself: The C standard does not allow enums for bit fields:

A bit-field shall have a type that is a qualified or unqualified version of _Bool, signed int, unsigned int, or some other implementation-defined type. It is implementation-defined whether atomic types are permitted.

Enum types are supported for bit fields in C++:

A bit-field shall have integral or enumeration type (3.9.1). It is implementation-defined whether a plain (neither explicitly signed nor unsigned) char, short, int or long bit-field is signed or unsigned.

[mayeut]

I think that having a type OPJ_BITFIELD allows to see at first glance that it is a bitfield.
Other than that, I don't see any advantage using a new type since unsigned int exists on all C compilers.

@detonin for feedback

[stweil]

Instead of unsigned int, you can also use the shorter unsigned which I personally prefer.

[mayeut]

Yes I know but some don't... that's why I always use unsigned int when unsigned is enough (and shorter as you mention)

[smuehlst]

I moved the definition of OPJ_BITFIELD from openjpeg.h to opj_includes.h.

[detonin]

I think it's a good idea to make immediately clear what type of variable is expected so I think OPJ_BITFIELD is good, and it actually does not harm. But defining it in opj_includes.h seems a bit weird to me (although I agree that if it's an internal type it should not modify openjpeg.h). Maybe we should at least define a new section in opj_includes.h with "Internal types" or sth like this. Comments ?

[stweil]

I agree that replacing OPJ_UINT32 by OPJ_BITFIELD improves the current situation for bit fields, because it fixes potential standard violations.
I disagree that using a single macro name for all kinds of bit fields would be a good idea because it does not support signed or boolean types for bit fields. If we want to cover all possible types of bit fields with macro names, we need for example OPJ_UINT_BITFIELD (= unsigned or unsigned int), OPJ_SINT_BITFIELD (= int) and OPJ_BOOL_BITFIELD (= bool). As far as I could see, most bit fields in the code are used for boolean values.

[mayeut]

@stweil, as you mention, all bitfields are used for boolean values in the code.
However bool type (which is in fact _Bool) only exist since C99 so not always usable here.
I guess to get true meaning at first glance, OPJ_BOOL_BITFIELD makes sense but defined as unsigned int... This means there won't be any "semantic" checking by the compiler: one could do OPJ_BOOL_BITFIELD member:3 with no warning. Unless if it's defined as _Bool when C99 is supported (all slaves on travis CI). Given that both versions will be checked by CI (>=C99 on unix/ < C99 on windows), I think it could be OK. Any thoughts ?

@detonin,

Maybe we should at least define a new section in opj_includes.h with "Internal types" or sth like this

with only one typedef for the moment, I think what @smuehlst is enough for now.

[stweil]

As the current code already uses bool (in C and in C++ files), that type can be used for new code, too (maybe stdbool.h has to be included if it is missing).
bool member:3 is valid code which won't result in a compiler warning. Using 8 or 16 bits for a bool value can even be reasonable because reading and writing bytes or words is faster than accessing single bits. bool can make a difference in conditional statements and in assignments, and there compilers can also show warnings.
We could try OPJ_BOOL_BITFIELD set to bool and look whether the critical compilers (IBM iSeries, Windows) complain. Although I still think that using standards is better than project specific hacks: they were needed in the early years of C programming but that's no longer generally true since at least 10 years. Just think of conversions from OBJ_BOOL (int) to bool or OPJ_BITFIELD (unsigned) and the possible programming errors which cannot be detected by the compiler simply because you don't use the type which was designed for that purpose.

[mayeut]

As the current code already uses bool (in C and in C++ files), that type can be used for new code, too (maybe stdbool.h has to be included if it is missing).

The only files were bool is used are for OPJViewer & openjp3d projects so out of scope for the discussion regarding openjp2 which shall compile with C89 compilers.
stdbool.h is C99 and MSVC does not provide it (MSVC now provides stdint.h - VS2010 - & inttypes.h - VS201?)

bool member:3 is valid code which won't result in a compiler warning. Using 8 or 16 bits for a bool value can even be reasonable because reading and writing bytes or words is faster than accessing single bits. bool can make a difference in conditional statements and in assignments, and there compilers can also show warnings.

Well, bool member:3 is invalid in C99. Hard error with clang:
clang test-bool.c outputs

test-bool.c:5:8: error: width of bit-field 'first' (3 bits) exceeds width of its type (1 bit)
  bool first:3;
       ^
1 error generated.

The file test-bool.c:

#include <stdio.h>
#include <stdbool.h>

struct myboolstruct {
  bool first:3;
  bool second:1;
};

int main() {
  struct myboolstruct t;

  t.first = 2;
  t.second = 0;

  printf("first: %d - second: %d\n", t.first, t.second);

  return 0;
}

As you stated, the underlying type for bool is of course wider than 1 bit. Nevertheless, it's forbidden to create bitfield members with width larger than one bit (makes sense to me). This is true for C99 and later. Don't know for C++.

We could try OPJ_BOOL_BITFIELD set to bool and look whether the critical compilers (IBM iSeries, Windows) complain. Although I still think that using standards is better than project specific hacks: they were needed in the early years of C programming but that's no longer generally true since at least 10 years. Just think of conversions from OBJ_BOOL (int) to bool or OPJ_BITFIELD (unsigned) and the possible programming errors which cannot be detected by the compiler simply because you don't use the type which was designed for that purpose.

Well, I agree with the general idea however compatibility with C89 is mandatory... That's why I proposed the mixed approach to define OPJ_BOOL_BITFIELD given that CI jobs will check that both paths are executed the same way. Use proper standard if available, else use "wrong" type (we're still in the early years of C programming...). The whole being seen as a ugly hack.

[stweil]

Thanks for that update. It is indeed different for C and C++. While gcc fails to compile bool val:3; and shows an error message ("width exceeds its type"), g++ accepts the same code.
So let's fix the current code, either with OPJ_BITFIELD, OPJ_BOOL_BITFIELD or maybe even with OPJ_BIT (because obviously our bit fields are always only a single bit).

[smuehlst]

@mayeut: Do I understand it correctly that the current code with OPJ_BITFIELD is "good enough" for the moment, or are additional changes necessary before the pull request will be accepted?

[detonin]

@smuehlst I suggest we merge this as is.