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

[Reimar Döffinger]

On Wed, Jan 14, 2009 at 01:28:27PM +0900, KO Myung-Hun wrote:
> Reimar Döffinger wrote:
>> 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
>>   
>
> Why do you always assume in that way ? I know, all the compiler for OS/2 
> supports '#pragma pack' directives, and so do they in the future.

And why do you want to make it hard to support different compilers?
IMO it is clearly you who has to give good arguments to use badly
portable constructs, not the other way round.

>> 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.
>>   
>
> Why do 'packed structs' make debug to hard ? And why does a single struct 
> need to be packed explicitly ? I think you missed '#pragma pack()' in the 
> end of file.

No, I did not miss the '#pragma pack()' at the end (which btw. is wrong,
but more on that later).
But for debugging I sometimes include a header file in the middle of the
file. All structs declared in that header file might have wrong packing
now.
Another way of reasoning: pack is a hack, and hacks should be only
applied where they are needed, not on a whole file.
Now to why '#pragma pack()' is wrong:
In the following code:
> #pragma pack(1)
> #include "vcd_read_os2.h"
> struct somestruct {
>   char a;
>   int b;
> };

somestruct will not be packed. Obviously I consider both the '#pragma
pack(1)' and the include after it bad style, but still the header has
one more annoying side-effect.
The correct usage especially in headers is
#pragma pack(push, 1)
and
#pragma pack(pop)

>> in the case of sParamDisk, the struct is completely pointless and makes
>> the initialization needlessly hard to read.
>
> In that case, we can use a byte buffer. But using struct is not a bad 
> choice. It's just a problem of favor. And I wanted to maintain consistency 
> if possible.
>
> Why hard to read the initialization ? I don't understand.

I think that {{'C', 'D', '0', '1'}} is harder to read than "CD01" or
{'C', 'D', '0', '1'}.

>> In the case of sParam and sParamTrack the struct type declaration code
>> is duplicated, and I don't agree that
>>   
>
> Hmmm... Why should not the declaration code be duplicated ? That cause any 
> problem ? Or just do you dislike it ?

Because code should not be duplicate if avoidable in general?
Also, I find it inconsistent that you argue it is important that the
struct allows you to give names to the members but you do not give the
structs any names, nor are the variable names like sParam particularly
helpful.

>>>    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};
>
> What do you mean by 'readability' ?

Well, mostly 5 lines of code vs. 1 line of code. In relative terms that
is a huge difference.
Also if you rename the loop variable "i" to "track" it has a proper name
just like the struct.
Though I admit the biggest advantage is due to the declaration being
where it is actually used - which of course can be done with the struct,
too.

>> 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.
>
> If so, which is more meaningful and readable between sParamTrack.bTrack and 
> track[4] ?

Well, but in that case the decision would be between

sParamTrack.bTrack = i;
and
uint8_t sParam[5] = {'C', 'D', '0', '1', track};

> Are you sure that all the compiler support per-struct declaration ? 
> Actually, 'packed' attribute is ignored on GCC port for OS/2, but all the 
> compilers for OS/2 accept '#pragma pack' directive.

Ok, very weird. That changes things of course. I'd still consider it
better style to either use packed around each struct individually
or do one block of
#pragma pack(push, 1)
struct a {...};
struct b {...};
#pragma pack(pop)

and then use those named structs (and get rid of that struct that
contains only a single array, I really see no point in using a struct
there...).

Greetings,
Reimar Döffinger