Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bit fields type #805

Merged
merged 4 commits into from
Sep 13, 2016
Merged

Bit fields type #805

merged 4 commits into from
Sep 13, 2016

Conversation

smuehlst
Copy link
Contributor

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.

smuehlst and others added 2 commits July 25, 2016 20:46
The definition of bit-fields with type OPJ_UINT32 caused complilation errors
on IBM iSeries, because OPJ_UINT32 is defined as uint32_t, and
uint32_t is defined as unsigned long in <stdint.h>. The definition of
bit-fields with an integer type of  a specific size doesn't make sense
anyway.
@@ -129,6 +129,8 @@ typedef uint64_t OPJ_UINT64;

typedef int64_t OPJ_OFF_T; /* 64-bit file offset type */

typedef unsigned int OPJ_BITFIELD;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@smuehlst smuehlst Sep 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@stweil
Copy link
Contributor

stweil commented Sep 6, 2016

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
Copy link
Contributor

stweil commented Sep 6, 2016

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

@stweil
Copy link
Contributor

stweil commented Sep 6, 2016

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
Copy link
Contributor Author

smuehlst commented Sep 6, 2016

@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
Copy link
Contributor

stweil commented Sep 6, 2016

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
Copy link
Contributor

stweil commented Sep 6, 2016

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
Copy link
Collaborator

mayeut commented Sep 6, 2016

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
Copy link
Contributor

stweil commented Sep 6, 2016

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

@mayeut
Copy link
Collaborator

mayeut commented Sep 6, 2016

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

Stephan Mühlstrasser added 2 commits September 7, 2016 08:35
OPJ_BITFIELD is used only in internal headers and must not
appear in the public openjpeg.h header.
@smuehlst
Copy link
Contributor Author

smuehlst commented Sep 7, 2016

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

@detonin
Copy link
Contributor

detonin commented Sep 7, 2016

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
Copy link
Contributor

stweil commented Sep 7, 2016

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
Copy link
Collaborator

mayeut commented Sep 7, 2016

@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
Copy link
Contributor

stweil commented Sep 7, 2016

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
Copy link
Collaborator

mayeut commented Sep 7, 2016

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
Copy link
Contributor

stweil commented Sep 8, 2016

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
Copy link
Contributor Author

@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
Copy link
Contributor

detonin commented Sep 13, 2016

@smuehlst I suggest we merge this as is.

@detonin detonin merged commit 8750e18 into uclouvain:master Sep 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants