On Fri, Nov 09, 2007 at 04:00:23PM -0000, SVN commits to the Asterisk project
wrote:
> Author: murf
> Date: Fri Nov 9 10:00:22 2007
> New Revision: 89129
>
> URL: http://svn.digium.com/view/asterisk?view=rev&rev=89129
> Log:
> This is the perhaps the biggest, boldest, most daring change ...
I have no objection with this specific change and the features it
adds/modifies, but I am very concerned with the way this is being brought in.
This (and a number of other asterisk source files) are already
longer than reasonable. Adding the entire matching code inline,
instead of using a separate file, makes the code even worse from
the point of view of maintaining it.
What you should have done is:
- define a suitable api for extension matching (functions to be called,
perhaps extra arguments in the ast_context);
- add the extra arguments as void * to ast_context;
- define global function pointers for the various matching functions,
assigning them by default to the current implementation;
- put the new extension matching code in a new file, e.g. pbx_match2.c,
which is linked with the existing pbx.c code;
- add a config file option (and perhaps cli method) to select which
method you want to use at runtime.
This is not a lot of work (i did the same when i introduced the new
"say" functions), especially because at a quick look your code seems
well documented and structured, so it lends well to the modifications
i am suggesting.
What you gain is that the code becomes a lot more manageable, and
it is a lot less risky for people to test trunk, because there is
a trivial way to switch back and forth between the two implementations
and compare behaviour.
I hope you will follow this suggestion.
cheers
luigi
_______________________________________________
--Bandwidth and Colocation Provided by http://www.api-digital.com--
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
