[MPlayer-dev-eng] [PATCH] VCD support for OS/2
KO Myung-Hun
komh at chollian.net
Fri Jan 16 04:32:02 CET 2009
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
More information about the MPlayer-dev-eng
mailing list