jphickey
commented
2 years ago This is actually a portability bug because the bit flags in "all" are not guaranteed to be the same actual bits as the flags in the rx/tx union members. So when crossing between the two union members, undefined behavior may result. The linked PR addresses this.
Use of bitfields is discouraged by many coding standards (including GSFC) because the C standard does not specifically dictate how they are packed into the underlying integer type. CF uses them in several internal structures, for example:
https://github.com/nasa/CF/blob/2ca7f978aea855ba4d6adc6e5370cbb2178129a5/fsw/src/cf_cfdp.h#L159-L165
If these truly need to be bit fields, then they should be implemented explicitly using shifts and masks. However, initial inspection of the code would suggest they do not need to be bit fields, they can be made into separate fields. While this may increase the memory footprint somewhat (struct is likely to be 5 bytes in the example instead of 4) this is probably a reasonable trade, because separate fields can be simply read/written directly rather than requiring a read-modify-write etc.
Note that when using bit fields, the assembly instructions to do shifts and masks will still be generated by compiler, even though the C syntax "looks" simple - it is hiding it all. So it may be considerably less efficient than accessing separate memory locations. This of course depends on hardware architecture, caching, optimization by the compiler, etc but in general bitfields will always be less efficient, due to the extra shifting and masking.