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

[KO Myung-Hun]

Hi/2.

Reimar Döffinger wrote:
> 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.
>
>   

Huh... Would you mind telling me which compiler does not support 
'#pragma pack', especially on OS/2 ? Please....

>>> 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)
>
>   

Ah, ok. You should have said these at first. Frankly I thought of that 
problem but I didn't know that feature of '#pragma pack'. I learned the 
new and good thing due to you.
Thanks, Reimar. ^______^

>>> 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'}.
>
>   

First of all, "CD01" is not the case, because that field is not a 
null-terminated. And so harder to read {{'C', 'D', '0', '1'}} than {'C', 
'D', '0', '1'} ? Hmm.... Anyway I accept it.

>>> 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?
>   

Huh ? Really ?  I think C&P is my friend ^^, and it depends on a 
situation. For example, unrolling.

> 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.
>   

Should I give a name to a struct ? And 'sParam' itself means 'param' arg 
of DosDevIOCtl(). However in vcd_read_toc(), DosDevIOCtl() is called 
twice with different function codes, so I gave different names to each 
struct variables to distinguish them. Any problem ?

<snip>

>> 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...).
>
>   

I have a question. Are there any differences between named struct and 
unnamed struct ? Does it decrease performance ? And it's ok to get rid 
of the struct containing a single array as I said above.

BTW, I think there are many ways to express the same thing, and we 
cannot say which one is superior to the other. If performance is same, 
forcing to use only one way is not good. For example, one can prefer 
pointer style, but the other can prefer array style. Respect the style 
of each programmer if possible, please.

-- 
KO Myung-Hun

Using Mozilla SeaMonkey 1.1.14
Under OS/2 Warp 4 for Korean with FixPak #15
On AMD ThunderBird 1 GHz with 512 MB RAM

Korean OS/2 User Community : http://www.ecomstation.co.kr


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: vcd.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090114/405d08ec/attachment.asc>