[FFmpeg-devel] [PATCH] mxfdec: make it work with other calling conventions

[Reimar Döffinger]

On Wed, Jun 30, 2010 at 12:01:08AM -0700, Baptiste Coudurier wrote:
> On 6/29/10 10:25 PM, Reimar D?ffinger wrote:
> >>>>>+#define METADATA_READ_FUNC(name) int name(void *arg, ByteIOContext *pb, int tag, int size, UID uid)
> >>>>
> >>>>I don't like the #define.
> >>>
> >>>Well, the alternative is having to change every single read function
> >>>manually in case you'd ever need an additional parameter.
> >>>And I found it very hard to find them.
> >>
> >>And when you add a new function, you have to look up for the define
> >>to know which parameters are available. This is even more painful.
> >
> >Have you actually _tried_ changing the parameters of all those functions?
> >
> >I had to pick each single one from the table, search for it's declaration
> >and then change it.
> >Searching for the define to get the argument names is quite simple in
> >comparison.
> >But you have to maintain it, I'll change it if that's the only objection.
> >
> 
> I do know for sure that I've been working way more with that file
> than you. Please remove the define.

This wasn't about this file in general, but the annoying task of changing
the function arguments.
I thought I had found a really nice way to avoid that pain in the future.
Though since they are now all identical I guess sed works reasonably well
now, too.

> >          for (metadata = mxf_metadata_read_table; metadata->read; metadata++) {
> >              if (IS_KLV_KEY(klv.key, metadata->key)) {
> >-                int (*read)() = klv.key[5] == 0x53 ? mxf_read_local_tags : metadata->read;
> >-                if (read(mxf,&klv, metadata->read, metadata->ctx_size, metadata->type)<  0) {
> >+                int res;
> >+                if (klv.key[5] == 0x53) {
> >+                    res = mxf_read_local_tags(mxf,&klv, metadata->read, metadata->ctx_size, metadata->type);
> >+                } else
> >+                    res = metadata->read(mxf, s->pb, 0, 0, NULL);
> >+                if (res<  0) {
> >                      av_log(s, AV_LOG_ERROR, "error reading header metadata\n");
> >                      return -1;
> >                  }
> >
> 
> What you do is only cosmetic changes. Please keep the code the way
> it is. If we need to change the prototype because a feature requires
> it, we will change it.

(removed for now in below patch despite following objections)
It's justified to argue the practical relevance, but calling it
"cosmetic" when one is valid C and the other is not seems a bit extreme.
Nothing I know of the C standard allows you to call a function
with
pointer, pointer, int, int, pointer
signature with arguments
pointer, pointer, pointer, int, int
even though I know of no calling convention where it actually causes issues.
The bigger problem from my point of view is the use of the () declaration syntax,
which breaks quite a few assumptions people make.
E.g. for
int foo();

foo(0);
and
foo(NULL);
are not the same thing, and for e.g. stdcall on 64 bit at least one will crash.
Or in this specific example, if someone changes the type of metadata->ctx_size
without changing the parameter of mxf_read_local_tags in the same way, it
will break as well.
So for once, I consider M?ns' short version of it not an exaggeration at all.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mxfdec.diff
Type: text/x-diff
Size: 4514 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100630/9b687d77/attachment.diff>