[MPlayer-dev-eng] [PATCH] VCD support for OS/2

[Reimar Döffinger]

On Wed, Jan 14, 2009 at 01:10:47AM +0900, KO Myung-Hun wrote:
> On OS/2, DosDevIOCtl() expect a packed struct or a byte buffer. So I 
> decide to use '#pragma pack' globally because all structs are for 
> DosDevIOCtl() except 'mp_vcd_priv_t'. And the system headers for them do 
> not exist.

So with pragma you now have:
1) specified packed in a way that a (hypothetical) compiler which does
not support it will most likely ignore, thus failing at runtime instead
of compiletime
2) made all structs declared in the file packed, which is currently
not really good in case of mp_vcd_priv_t and can cause hard to debug issues if
someone is ever so careless to include a .h file after the pragma,
while at the same time only a single struct actually _needs_ to be
packed explicitly.

in the case of sParamDisk, the struct is completely pointless and makes
the initialization needlessly hard to read.
In the case of sParam and sParamTrack the struct type declaration code
is duplicated, and I don't agree that
>    struct {
>       UCHAR   auchSign[4];
>       BYTE    bTrack;
>    } sParamTrack = {{'C', 'D', '0', '1'},};
[...]
> sParamTrack.bTrack = i;

is better for readability than one

> uint8_t track[5] = {'C', 'D', '0', '1', i};

Which also avoids the assumption that DosDevIOCtl will not modify this
parameter, it is not marked as "const" in the function prototype after
all.

> Of course, I absolutely agree with you if these codes are for general 
> platforms and OSes. But these are only for OS/2. And those structs are 
> defined in according to OS/2 API DOCs. Why should I consider what you said ?

Are those OS/2 API docs publicly available somewhere?

> And I don't understand a byte buffer has a better readability than a 
> structure. If not using a structure, how can I know the meaning of each 
> index ?

How can you know the meaning with names like auchSign? I consider the
non-struct version as in the example I gave above more readable.

> And just a question. Is there a compiler not supporting 'packed structure' ?

Probably not, but as I remember it early gcc versions did not support
#pragma pack, in general each compiler tends to have their own way of
specifying it - per-struct declarations usually can be fixed by using a
macro instead, whereas #pragmas generally either work or they don't and
usually there is no way to even find out.

> Yes, many compiler set the default stack size for OS/2 to 32K or 64K. 
> But I think that size is enough to use 2K stack variable. In addition, 
> GCC 4.3.2 are being ported to OS/2. It set the default stack size to 1M. 
> Currently I'm using that GCC 4.3.2. Nevertheless I'm setting the stack 
> size to 16M for MPlayer. So I think your apprehension is unnecessary.

And why not just put it in the mp_vcd_priv_t that you already have
anyway, thus making sure it will _certainly_ work for "any" stack size?

Greetings,
Reimar Döffinger