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

[KO Myung-Hun]

Hi/2.

KO Myung-Hun wrote:
> 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.
>

No more comments ?

If so, apply 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