Hi Alexander,
On Thu, Nov 24, 2011 at 10:36:27AM +0100, Alexander Huemer wrote:
On Thu, Nov 24, 2011 at 07:58:33AM +0100, Sylvain Munaut wrote:
Hi,
First, thanks for this series of patch, I'll be sure to review and test them.
Well, no problem, I had some time on hand I couldn't use otherwise.
this eliminates the occurrance of gcc warning warning: cast increases required alignment of target type
src/target/firmware/include/comm/timer.h | 4 +++- src/target/firmware/include/layer1/sched_gsmtime.h | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-)
-}; +} __packed;
I'm not conviced by this one : Why should we use packed for structures not used as 'communication packets' ?
What are the drawbacks ?
normally, the compiler will lay out the structure in a way that optimizes accesses to members. So e.g. on an ARM, a
struct { uint8_t byte1; uint8_t byte2; };
will very likely contain 3 bytes padding between the two uint8 values in order to make sure no unaligned loads/stores will be required.
However, if you change that to '__packed', the padding will not be generated and any access to struct members will need to deal with unaligned accesses, which can be inflating the code size considerably.
Steve and Holger raised some reasonable concerns on some of the other patches, it seems I was quite unconcentrated and they are mostly crap.
don't say that. We love to receive cleanup patches like yours, and even if 2-3 have some issues, at a series of 8 it is still considerable gain for the project.
The patches do not contain any "magic" anyway. Just straight-forward reaction on the different kind of warnings. Most of them can be eliminated easily.
yes, in most cases. However, sometimes we keep the warning around as a reminder that something still needs to be done, such as e.g. handling an undhandled enum value in switch() or the like. In such a case of course we shuold implement the missing member, and not introduce a default case.
Regards, Harald