On Monday 15 September 2008, Francesco Romani wrote: > On Mon, 2008-09-15 at 08:26 +0200, Georg Martius wrote: > > Hi, > > Hi, thanks for your patches! > > > please find the patch for moving the code to the coding style. Created > > with cvs diff in the filter/stabilize directory. > > Yup, applied. Mostly OK, but there is still some code that should be > updated. No hurry for that. Okay thanks! Sorry, I must have overlooked something. Also to be honest I checked other parts of transcode and it is partly not very close to the style guide ;-) One thing I was not sure about was whether function documentation goes in .h and .c file or only in one place. I myself would put in .h ( at least for the public interface) and the rest in .c. However I found in the other code mostly the comment block in the .c file, so that's what i did. Duplicating comment is in my eyes a bad idea because it will be outdated very quickly.
> > I also abstracted the linked list implementation as earlier suggested.
> > Since the file is not yet in the CVS it is attached too. Eventually this
> > might be moved to libtc.
>
> Yes, I plan to add it to libtc because we already have at least another
> piece of code (cfgfile.c) needing it a list implementation.
>
> I'll spend some time thinking about the API which fit best with TC
> ecosystem, but it will not be very different from yours linkedlist.
Okay, I am happy to change my code to a new API. My implementation copies the
entries, not to have unwanted side-effect and to handle cleanup easily. For
my purpose it is really no problem. In case you want to avoid the extra copy
cost I see suggest to provide two APIs :
1) the easy interface (as it is) and
2) one for handling memory at the caller side with some descriptive suffix
like linkedList_add_insitu(...) and linkedList_delete_withdata() (same as
in 1), linkedList_delete_withoutdata() and so on
> > I have some questions:
> > How is the help for filters supposed to work? I figured that I can use
> > tcmodinfo. But filters like filter_levels suggests that I can pass the
> > option help and it will print the help message, but it does not.
>
> For new-style modules (even if used through compatibility layer, as for
> filters), the former behaviour has changed. To query the module
> parameters, or to see the default values of them, the user must use the
> tcmodinfo tool:
>
> $ tcmodinfo -i levels
>
> for configuring parameters, the module (of course) still accept options
>
> $ transcode [...] -J levels=pre=1
>
> The core idea is that one shouldn't be _forced_ to perform fake runs of
> transcode just to inspect the module characteristics: use tcmodinfo (as
> the executable name suggests!) separately instead.
>
Okay I understand that but I still think
a) it would not harm to print the parameter help when -J filtername=help is
used, and it should be no problem to implement at the pluginhandler side.
b) if this will not be provided, then all the filter documentation has to be
changed to not mention the "help" option.
c) tcmodinfo should be documented for the user e.g. if transcode --help is
called.
> > I am also confused about the logging and verbosity.
> > The functions tc_log_XXX do not filter but only format. That means that I
> > always have to write if(verbose & TC_INFO) tc_log_info(.....)?
>
> Yes you shall go this way until tc_log_* infrastructure receive further
> enhancements (the timeframe is next major release).
>
> > What about changelog and todos for the plugin?
>
> Just embed them in the main source, there is no official guidelines for
> that, every module goes on it's own way.
So in principle I could keep my own list of Todos and stuff in the stabilize
filter directory?
> > I am happy for further comments.
>
> I'm still reading your code, so I haven't much more to say :)
>
> Just a word of warning: while I _personally_ like glib/gtk-isms
> (oop-isms?) like the following
>
> #define NEW(type,cnt) (type*)tc_malloc(sizeof(type)*cnt)
>
> there is _NO_ general consensus about such things in the transcode-dev
> environment, nor anyone like them. So it is best to avoid them.
>
> PS: the cast after the malloc is useless and it should be removed
I am sorry I didn't know that. NP to remove that. Why is the typecast useless,
malloc as return type (void*)?!
Regards,
Georg
signature.asc
Description: This is a digitally signed message part.
