Trent Piepho schrieb:
On Fri, 19 May 2006, Marcel Siegert wrote:
Trent Piepho wrote:
On Fri, 19 May 2006, Manu Abraham wrote:
Trent Piepho wrote:
Why is there padding in both the xxxx_params structs, and in the union that
contains them? Same for the xxxx_info structs and the union they are in. If
you have the padding value in the union, the padding in each of the structs in
the union isn't necessary.
Well, it was a suggestion from Johannes. pad[32] and pad [128]
I think maybe that isn't what he meant. Maybe Johannes can provide
reasoning why structures in a union and the union itself all need padding.
the might be 2 reasons:
a) don't get any kind of e.g. memcopy problems with new structs and old kernels after
this patch went in. so the size of e.g. the tuning params for dvb-s2 can be extended
without having to change the structs size. so also old kernel will perform a correct
memcopy.
b) the union as a member of the params struct is a large as its biggest member.
so we just reserve some space, because we do not know actually, how e.g. DVB-C2/T2 or
something similar will use as its struct size. as size of the params is used to
calculate the ioctl itself, just adding a new struct that has a size 1 byte more than
the actual reserved would cause backwards incompatibility in future. (this is the
actual trapdoor we felt into and that we want to prevent off in the future)
That is a good reason for having padding. I'm not saying, get rid of all the
padding. Some padding should be kept for just those reasons. But padding
inside each struct, and in the union those structs are in, isn't necessary.
Look at all the dvbfe_xxx_info structs. They all have 32 bytes of padding.
This means they are all bigger than 32 bytes, right? Then the union they are
in has 32 bytes of padding. But all the structs are bigger than 32 bytes, so
this padding does nothing!
i assume that was a copy&paste bug. you are right on that specific topic on that specific
struct.
enum dvbfe_interleaver {
DVBFE_INTERLEAVER_IGNORE = (0 << 0),
DVBFE_INTERLEAVER_NATIVE = (1 << 31),
DVBFE_INTERLEAVER_INDEPTH = (1 << 30),
DVBFE_INTERLEAVER_AUTO = (1 << 29)
};
Now the DUMMY value isn't needed. One less symbol is defined, and there is
no issuse with warnings on switch statements. No one needs to look at the
value of DVBFE_INTERLEAVER_NATIVE, so who will care that it is now (1<<31)
instead of (1<<0)?
the dummy value is needed to keep struct sizes on different architectures. e.g. arm
Yes, I know that. But if you look at that definition above, it starts
numbering with the most significant bits, so one of the enums has the value
1<<31. This way an extra dummy value isn't needed.
my 0.02$:
and as, from my knowledge, counting starts at 0 in a computer environment/programming language
normally. so your idea is practicable to avoid a value, but non standard in normal coding
logic. that even may confuse people more than having a specialised dummy value.
You could also say that allocating bits starting at the most significant is
the natural order too. But why does it matter? No one needs to look at the
values, so who is going to be confused? Assuming that the sequence 31, 30,
29, 28, 27, 26 is going to confuse anyone. Do you really think anyone could
look at that sequence and not figure out what number comes next?
yes, sometimes you may just have in mind that XXX_IGNORE is first == 0 and XXX_AUTO is last valid part
so you may use a
switch (onWhatever)
{
case XXX_IGNORE ... XXX_SOMEWHERE_IN_THE_MIDDLE:
{ /* act here like you want to */ }
break;
case XXX_ON_MORE_THAN_SOMEWHERE_IN_THE_MIDDLE ... XXX_AUTO:
{ /* act here like you want to */ }
break;
default:
printf("BUG! you must have missed a value!\n");
break;
}
if we follow your numbering scheme the compiler would complain about an empty range,
if it is designed in the actual way manu did, everything is fine.
regards
marcel
_______________________________________________
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
